oauth2: tighten regression tests for spurious-success decrypt fix

[Copilot]

The two regression tests added for the AES-CBC spurious-success crash fix were asserting on "failed to decrypt token" — a prefix shared with the pre-existing EVP_DecryptFinal_ex failure branch — so they would pass even if the new headerValueIsValid guard were regressed away.

Changes

• DecryptTokenSpuriousSuccessReturnsOriginalInput: Narrow EXPECT_LOG_CONTAINS to "plaintext is not a valid header value", which is unique to the new branch (decrypt_result.error.value_or("plaintext is not a valid header value")). A revert of the fix hits the old path and emits a different message, failing this test.

• GarbagePlaintextCookieDoesNotCrash:

  • Same log-message narrowing.
  • Replace bare cookies.at("RefreshToken") with ASSERT_TRUE(cookies.contains("RefreshToken")) + EXPECT_EQ(...) so a dropped cookie fails clearly rather than throwing out_of_range.
  • Add EXPECT_CALL on decoder_callbacks_.encodeHeaders_ to assert the redirect actually fires:

    EXPECT_CALL(decoder_callbacks_,
                encodeHeaders_(testing::Truly([](const Http::ResponseHeaderMap& headers) {
                                 return headers.getStatusValue() == "302" &&
                                        !headers.getLocationValue().empty();
                               }),
                               true));

    Checks only Status == "302" and Location non-empty; avoids matching Set-Cookie/nonce/CSRF values that would make the assertion brittle.

DecryptTokenEmpty, DecryptTokenSameSecret, and DecryptTokenDecryptionFails are unchanged — they provide the only direct unit-test coverage of filter_->decryptToken/encryptToken.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• dl.google.com
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)
• ftpmirror.gnu.org
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)
• mirror.bazel.build
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)
• pkgconfig.freedesktop.org
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Original prompt

Tighten the two regression tests added in PR #44677 (test/extensions/filters/http/oauth2/filter_test.cc) so they specifically verify the new fix path and not the pre-existing decrypt-failure path. Keep DecryptTokenEmpty and DecryptTokenSameSecret as-is — they cover the only direct unit-test coverage of filter_->decryptToken / filter_->encryptToken.

Required changes (only to the two tests below — do not change anything else)

1. DecryptTokenSpuriousSuccessReturnsOriginalInput

Currently asserts on the generic prefix "failed to decrypt token" which is shared with the pre-existing EVP_DecryptFinal_ex-failure branch. Change the EXPECT_LOG_CONTAINS to match on the message that is unique to the new branch:

"plaintext is not a valid header value"

That string comes from the decrypt_result.error.value_or("plaintext is not a valid header value") in source/extensions/filters/http/oauth2/filter.cc. Matching on it ensures this test only passes when the new headerValueIsValid branch fires — a regression that re-introduced the crash via the old error path would emit a different message and fail this test.

2. GarbagePlaintextCookieDoesNotCrash

Apply three changes:

• Tighten the log assertion the same way as above: match "plaintext is not a valid header value" instead of "failed to decrypt token".

• Defensive cookie lookup: replace the bare cookies.at("RefreshToken") with ASSERT_TRUE(cookies.contains("RefreshToken")) followed by EXPECT_EQ(cookies.at("RefreshToken"), garbage_ciphertext); so the test fails clearly (rather than throwing out_of_range) if the cookie were dropped instead of preserved.

• Assert the redirect actually happened: add an expectation that decoder_callbacks_.encodeHeaders_ is called once with a 302 response. Look at the existing test RedirectToOAuthServerWithInvalidCSRFToken in the same file for the established pattern (uses EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true));).

  Do NOT match the full response header map — that would be very brittle (Set-Cookie values, Location with random nonce/csrf, etc.). Instead, use a custom matcher (or a Truly(...) lambda) that only checks headers.Status() is "302" and headers.Location() is non-null. The intent is "filter took the redirect path rather than crashing or 401-ing", nothing more. If a simple _ match is the only realistic option without significant test infrastructure work, that's acceptable too — but document why in a comment.

  Note: redirectToOAuthServer currently calls decoder_callbacks_->encodeHeaders with end_stream=true, so the matcher's second arg should be true.

Things NOT to change

• Do not modify source/extensions/filters/http/oauth2/filter.cc.
• Do not modify changelogs/current.yaml.
• Do not modify DecryptTokenEmpty, DecryptTokenSameSecret, or DecryptTokenDecryptionFails. They cover branches that have no other direct unit-test coverage (verified via lexical search — there are no other callers of filter_->decryptToken / filter_->encryptToken in test/extensions/filters/http/oauth2/).
• Do not add new tests.
• Keep all existing comments/docstrings; you may extend them to explain why the tighter assertion is correct.

Verification

Build and run only the affected test target:

bazel test //test/extensions/filters/http/oauth2:filter_test --test_filter='*DecryptToken*:*GarbagePlaintextCookie*'

Both DecryptTokenSpuriousSuccessReturnsOriginalInput and GarbagePlaintextCookieDoesNotCrash must pass against the existing fix in filter.cc. As a sanity-check that the assertions are actually tighter, you can mentally (or via a temporary local revert) confirm: if filter.cc were reverted so the new branch did not exist and the old EVP_DecryptFinal_ex failure path were hit instead, the new log-message assertion would fail. Do not commit that revert.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: trying to track down a flake in oauth2 tests that are hard to repro (requires full test suite and doesnt always repro even with x100)

so - probs with this pr bigger q is - is it a real "bug", does this fix it, are all these tests necessary, and do they test correctly

Assistant: Let me also quickly check the encryptToken accessor (test calls filter_->encryptToken) and existing tests to see if these new tests overlap with existing coverage:

Assistant: # Review of PR #44677 — oauth2: fix crash in decryptToken on spurious AES-CBC PKCS#7 success

TL;DR

• ✅ Yes, this is a real bug. The crash mechanism described is correct and reproducible from first principles. Whether it explains your specific flake is another question (see below).
• ✅ The fix correctly prevents the crash. It uses the exact same predicate (HeaderUtility::headerValueIsValid) that HeaderString::assertValid() uses inte...

This pull request was created from Copilot chat.

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #44682 was opened by Copilot.

see: more, trace.