Avoid panic on malformed scan rules

This endpoint forwards untrusted request rules directly into build_sds_scanner, but scanner construction is panic-based (.build().expect(...) in crates/secrets/src/scanner.rs), so a malformed rule (for example an invalid regex pattern) can make /scan-secrets panic instead of returning a structured error response. Because scan_secrets also does .await.unwrap(), that panic propagates through the handler path and turns a bad client payload into a server-side failure rather than a normal validation error.

Useful? React with 👍 / 👎.

changed the signature of build_sds_scanner to return Result

This would be a bit more optimized if you checked the length of request.rules before attempting deserialization

Updated to check rules count before deserializing

Small nit, but I would probably rename this to text or data or something. Just helps to quickly communicate the intent (I am guessing we'll scan plain text as well)

Renamed field to data

I think Codex already caught this. It might be worth changing the signature of build_sds_scanner to return a Result. This method is only called in one place in the codebase besides here, so it is should be an easy change and would allow you to avoid dealing with a panic. It would only require a map_err to String in here.

The alternative would be to rely on Tokio catching the panic in the endpoint and then using unwrap_or_else to map it to a more appropriate error.

Since the latter approach depends on the panic configuration being unwind, I'd go for the first suggestion.

Agree we should fix it. I'd like to fix it in coordination with the code owner on a separate PR, to make sure he/she is aware and agrees with the change.

That makes sense, but then I would at least consider using here the unwrap_or_else alternative I mentioned in the endpoint. I'll leave my suggestion there.

PR updated, I changed the signature of build_sds_scanner to return Result

Given that we won't be refactoring the build_sds_scanner in this PR I think we should consider this in here:

.unwrap_or_else(|_| {
    json!(SecretScanResponse {
        rule_responses: vec![],
        errors: vec!["Internal error: scanner failed to initialize".to_string()],
    })
})

To be honest, I’d consider changing the build_sds_scanner signature in this PR since the impact is negligible. That said, I understand if you’d prefer to address it in a separate PR.

Agree with all of this

Ok, let's go with Plan A, to change build_sds_scanner signature. I'll update the PR

PR updated, changed the signature of build_sds_scanner to return Result