fix(web-bot-auth): fail closed on expired signatures in Rust verify

[SashaMIT]

Summary

WebBotAuthVerifier::verify accepted cryptographically valid signatures after expires, even though WebBotAuthError::SignatureIsExpired already exists and the TypeScript http-message-sig verifier rejects expired inputs before crypto verify.

This makes the Rust Web Bot Auth verifier fail closed on an expired (or unparsable) expires window, then proceeds to cryptographic verification only when the window is still valid. The sample under examples/rust/verify.rs is updated to match.

Advisory inspection via possibly_insecure remains available through get_parsed_label() for callers that want the soft check separately. Generic MessageVerifier::verify is unchanged.

Test plan

• cargo test -p web-bot-auth --lib (29 passed)
• cargo run -p rust-examples --bin verify
• CI green on this PR

Made with Cursor

[AkshatM]

I am torn about this one.

Fail-closed sounds intuitive and in line with offering fewer footguns, but it is actually not mandated by either the HTTP message signature IETF RFC or the web bot auth IETF RFC. In fact, https://www.rfc-editor.org/info/rfc9421/#name-enforcing-application-requi specifically calls this behaviour out as non-normative.

Further, it will almost certainly be a breaking change for consumers of the library. One of the hard parts about protocol behaviour in the wild is that once you've exposed some behaviour, people will rely on it. For verifiers, this means signatures that used to pass will break, which can affect a large swathe of users. (Whether they should have passed in the first place is another question...)

While grateful for the work, I am personally leaning towards respectfully declining this PR. I think it is better to have an implementation that adheres to the baseline specifications and allows applications to define their own non-normative behaviour rather than lock in all applications to an opinionated implementation. Verifiers can't control the signatures people send, and it is more important to be able to try to parse them and know when it is likely to be a bad idea than not to be able to parse them at all.

Open to others' thoughts. I recognize the Typescript variant does it differently, and interested in how much parity we want to maintain on this front. cc @thibmeu

[thibmeu]

Thanks for the PR. I agree with @AkshatM that this is better as a discussion than as a PR. I'd favor rejecting the signature as invalid as well (and failing close) by default. Having full validation with a warning that the signature is expired seems to be an option that needs to be passed, but not something that should be exposed by default.

I agree that neither RFC 9421 or the web-bot-auth draft clearly specify that application pattern. For the later, that's probably worth having a short text.

[AkshatM]

Having full validation with a warning that the signature is expired seems to be an option that needs to be passed, but not something that should be exposed by default.

I like this option, and, if implemented this way , I would be happy to approve and merge. Feel it addresses all components.

@SashaMIT - could you implement an optional behaviour to parse with warnings in addition to this change?

[SashaMIT]

Done, thanks both. verify keeps the fail-closed default (expired or unparsable expires returns SignatureIsExpired before crypto, matching the TypeScript verifier), and there's now an opt-in verify_with_advisory that always runs full cryptographic verification and returns AdvisoryVerification { timing, advisory }, so callers that need to parse and judge expired-but-valid signatures get the SecurityAdvisory as a warning instead of an error.

On the normativity point: agreed that RFC 9421 leaves this to application requirements, which is why enforcement stays opt-out rather than removed. The default keeps parity with http-message-sig, and the opt-in preserves the parse-everything behavior you described. @thibmeu happy to help with short text for the web-bot-auth draft if useful.

Tests: cargo test -p web-bot-auth --lib (30 passed, new regression test covers the advisory path on an expired vector) and cargo run -p rust-examples --bin verify demonstrates both paths. Also fixed an unclosed assert! paren in the example that CI doesn't build.

[AkshatM]

nit: change function name to verify_ignore_expiry. More clear about what this does.

[SashaMIT]

Renamed to verify_ignore_expiry as requested. Same behavior, just the clearer name. Tests still 30/30.

all comments fixed