test: add Go fuzz tests for OSS-Fuzz integration#4106
Conversation
Covers the core pre-auth token parsing boundaries in Ory Hydra: - HMAC token validation (access tokens, refresh tokens, auth codes) - HMAC signature extraction (token lookup path) - HMAC string generation (secret hashing) - JWT token validation (JWT access tokens, OpenID ID tokens) - JWT token decode (header/payload/signature parsing) - JWT signature extraction (token revocation path) These are the untrusted input boundaries for every OAuth2/OIDC request processed by Hydra. Tokens come from the Authorization header — fully attacker-controlled.
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a fuzz test suite ( ChangesHydra token cryptographic robustness testing
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fuzz_test.go`:
- Around line 4-17: Update the copyright header at the top of fuzz_test.go to
use the project-approved owner and SPDX tag: replace the Google LLC header block
with the exact lines "// Copyright © 2026 Ory Corp" and "//
SPDX-License-Identifier: Apache-2.0" so the file matches the repository's
required license/ownership format.
- Around line 184-189: The seed added via f.Add(string(make([]byte, 100000)))
exceeds the fuzz input cap checked in the Fuzz callback (len(token) > 1<<16), so
it will be discarded; change the seed to be at or below the cap (<= 1<<16 bytes)
or remove it entirely. Locate the f.Add call and replace the 100000-byte seed
with a smaller seed (e.g., make([]byte, 1<<16) or a smaller realistic input) so
it passes the size check in the Fuzz(func(t *testing.T, token string)) block.
- Around line 117-119: The fuzz harness for GenerateHMACForString lacks an
input-size cap and can be DoS'ed by huge strings; modify the Fuzz function
wrapping strategy.GenerateHMACForString to early-return or t.Skip when the
fuzzed string exceeds a safe length (e.g., 1KB–4KB) by checking len(text) and
skipping/returning before calling strategy.GenerateHMACForString, so the test
stays consistent with other fuzz functions and avoids memory exhaustion.
- Around line 100-102: The fuzz target f.Fuzz currently declares an unused
second parameter `compare` which wastes fuzzer cycles; update the anonymous
function signature in the f.Fuzz call to accept only the used `token` parameter
(e.g., change func(t *testing.T, token, compare string) to func(t *testing.T,
token string)) and then adjust any seed corpus entries used to call this fuzz
target so they no longer provide the obsolete second value; ensure you update
references around the f.Fuzz invocation and any helper/seed setup that assumed
two inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| f.Fuzz(func(t *testing.T, text string) { | ||
| _, _ = strategy.GenerateHMACForString(context.Background(), text) | ||
| }) |
There was a problem hiding this comment.
Add input size cap for consistency and DoS prevention.
Unlike the other fuzz functions in this file, FuzzHMACGenerateForString does not cap the input size. This could allow the fuzzer to generate very large strings that exhaust memory or slow down fuzzing.
🛡️ Proposed fix to add size cap
f.Fuzz(func(t *testing.T, text string) {
+ if len(text) > 1<<16 {
+ return
+ }
_, _ = strategy.GenerateHMACForString(context.Background(), text)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| f.Fuzz(func(t *testing.T, text string) { | |
| _, _ = strategy.GenerateHMACForString(context.Background(), text) | |
| }) | |
| f.Fuzz(func(t *testing.T, text string) { | |
| if len(text) > 1<<16 { | |
| return | |
| } | |
| _, _ = strategy.GenerateHMACForString(context.Background(), text) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fuzz_test.go` around lines 117 - 119, The fuzz harness for
GenerateHMACForString lacks an input-size cap and can be DoS'ed by huge strings;
modify the Fuzz function wrapping strategy.GenerateHMACForString to early-return
or t.Skip when the fuzzed string exceeds a safe length (e.g., 1KB–4KB) by
checking len(text) and skipping/returning before calling
strategy.GenerateHMACForString, so the test stays consistent with other fuzz
functions and avoids memory exhaustion.
| f.Add(string(make([]byte, 100000))) | ||
|
|
||
| f.Fuzz(func(t *testing.T, token string) { | ||
| if len(token) > 1<<16 { | ||
| return | ||
| } |
There was a problem hiding this comment.
Seed size exceeds fuzz cap.
The seed on line 184 adds a 100,000-byte string, but the fuzz function caps inputs at 64KB (1<<16 = 65,536 bytes). The seed will be immediately discarded by the size check.
♻️ Proposed fix to align seed with cap
- f.Add(string(make([]byte, 100000)))
+ f.Add(string(make([]byte, 1<<16)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| f.Add(string(make([]byte, 100000))) | |
| f.Fuzz(func(t *testing.T, token string) { | |
| if len(token) > 1<<16 { | |
| return | |
| } | |
| f.Add(string(make([]byte, 1<<16))) | |
| f.Fuzz(func(t *testing.T, token string) { | |
| if len(token) > 1<<16 { | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fuzz_test.go` around lines 184 - 189, The seed added via
f.Add(string(make([]byte, 100000))) exceeds the fuzz input cap checked in the
Fuzz callback (len(token) > 1<<16), so it will be discarded; change the seed to
be at or below the cap (<= 1<<16 bytes) or remove it entirely. Locate the f.Add
call and replace the 100000-byte seed with a smaller seed (e.g., make([]byte,
1<<16) or a smaller realistic input) so it passes the size check in the
Fuzz(func(t *testing.T, token string)) block.
canolgun-commits
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/cifuzz.yml (2)
33-34: 💤 Low valueSimplify the upload conditional logic.
Line 34 uses
if: always() && steps.build.outcome == 'success', which is redundant. Thealways()function forces the condition to be evaluated regardless of prior step outcomes, but thesteps.build.outcome == 'success'clause already enforces that the upload only runs if the build succeeded. Simplify toif: success()or the more explicitif: steps.build.outcome == 'success'.✨ Proposed conditional simplification
- if: always() && steps.build.outcome == 'success' + if: steps.build.outcome == 'success'Or if you want to always attempt upload (and let GitHub handle errors gracefully):
- if: always() && steps.build.outcome == 'success' + if: always()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cifuzz.yml around lines 33 - 34, The conditional for the "Upload Sarif" job step is redundant; replace the current "if: always() && steps.build.outcome == 'success'" with a simplified condition such as "if: success()" or, if you prefer explicitness, "if: steps.build.outcome == 'success'"; update the step named "Upload Sarif" in the workflow to use one of those simpler conditions so the upload runs only on successful builds.
13-16: ⚖️ Poor tradeoffConsider extending the sanitizer matrix to catch additional bug classes.
Line 16 defines a matrix with only the
addresssanitizer. While address sanitizer detects memory errors (heap/stack buffer overflows, use-after-free), expanding the matrix to includememoryandthreadsanitizers would catch data races and other concurrency bugs in token processing—important for auth infrastructure where race conditions can lead to authentication bypass.This is optional and defers to your threat model and CI resource budget, but given the criticality score (81/100, per PR context), comprehensive sanitizer coverage is recommended.
📊 Example: Extended sanitizer matrix
strategy: fail-fast: false matrix: - sanitizer: [address] + sanitizer: [address, memory, thread]This will run the fuzzer suite three times (once per sanitizer), multiplying CI time but providing deeper coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cifuzz.yml around lines 13 - 16, Update the CI sanitizer matrix to include additional sanitizers so fuzzing covers more bug classes: modify the strategy.matrix.sanitizer entry (currently only "address") to also include "memory" and "thread" (e.g., sanitizer: [address, memory, thread]) so the workflow runs fuzzers under AddressSanitizer, MemorySanitizer, and ThreadSanitizer; ensure CI resource implications are acceptable before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cifuzz.yml:
- Around line 4-5: The workflow's paths filter currently uses paths: ['**']
which triggers on every file change; replace that with a narrower set of glob
patterns targeting Go sources and fuzz tests (e.g. set paths to ['**/*.go',
'**/*_fuzz_test.go'] or ['**/*.go', '**/fuzz_test.go'] depending on your test
filenames) so the CIFuzz job only runs for Go code and fuzz test files; update
the paths: key in .github/workflows/cifuzz.yml accordingly.
- Line 20: Replace floating action refs with immutable commit SHAs: change
google/oss-fuzz/infra/cifuzz/actions/build_fuzzers@master and
google/oss-fuzz/infra/cifuzz/actions/run_fuzzers@master to specific commit SHAs;
also replace github/codeql-action/upload-sarif@v3 with a pinned commit SHA (or
align to `@v4` pinned SHA) in the workflow. While updating, narrow the
pull_request.paths field (currently pull_request.paths: ['**']) to only the
relevant paths that should trigger CI, verify strategy.matrix.sanitizer in the
job matrix (e.g., extend beyond only "address" if desired) and confirm
sarif_file: cifuzz-sarif/results.sarif matches the CIFuzz SARIF output naming
convention. Ensure you update the references for build_fuzzers, run_fuzzers,
upload-sarif, pull_request.paths, strategy.matrix.sanitizer, and sarif_file in
the workflow.
---
Nitpick comments:
In @.github/workflows/cifuzz.yml:
- Around line 33-34: The conditional for the "Upload Sarif" job step is
redundant; replace the current "if: always() && steps.build.outcome ==
'success'" with a simplified condition such as "if: success()" or, if you prefer
explicitness, "if: steps.build.outcome == 'success'"; update the step named
"Upload Sarif" in the workflow to use one of those simpler conditions so the
upload runs only on successful builds.
- Around line 13-16: Update the CI sanitizer matrix to include additional
sanitizers so fuzzing covers more bug classes: modify the
strategy.matrix.sanitizer entry (currently only "address") to also include
"memory" and "thread" (e.g., sanitizer: [address, memory, thread]) so the
workflow runs fuzzers under AddressSanitizer, MemorySanitizer, and
ThreadSanitizer; ensure CI resource implications are acceptable before
committing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dca7c4b5-1bba-46e3-b9b1-1871ba93a843
📒 Files selected for processing (1)
.github/workflows/cifuzz.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/cifuzz.yml (1)
1-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix workflow formatting to unblock CI.
The formatting pipeline is currently failing on this file (quote style + indentation diffs). Please run/apply the repository formatter so this workflow is stable in CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cifuzz.yml around lines 1 - 40, The YAML workflow has inconsistent quoting and indentation causing the formatting pipeline to fail; run the repository YAML/formatter and normalize quotes and indenting for the whole file (ensure top-level keys like "name: CIFuzz", the jobs block "jobs: fuzzing", and all steps such as "Build Fuzzers (${{ matrix.sanitizer }})", "Run Fuzzers (${{ matrix.sanitizer }})" and "Upload Sarif" use consistent indentation and quote style), then commit the formatted .github workflow so CI passes.Source: Pipeline failures
♻️ Duplicate comments (1)
.github/workflows/cifuzz.yml (1)
21-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse full 40-character commit SHAs for action pinning.
Line 21, Line 27, and Line 36 are pinned to short SHAs, which are not immutable-enough for supply-chain hardening. Please pin each action to a full commit SHA.
#!/bin/bash set -euo pipefail echo "Checking action refs in .github/workflows/cifuzz.yml..." awk ' /uses:/ { ref=$0 if (match(ref, /@[0-9a-f]{40}([[:space:]]|$)/)) { print "OK : " ref } else { print "WARN : " ref } } ' .github/workflows/cifuzz.ymlAlso applies to: 27-27, 36-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cifuzz.yml at line 21, Replace the short commit SHAs used in the workflow action pins with full 40-character commit SHAs: locate the uses entry "google/oss-fuzz/infra/cifuzz/actions/build_fuzzers@ba0e2e0" and the other two uses: the entries flagged on lines 27 and 36, fetch the corresponding full 40-character commit SHA from each action's GitHub repository (e.g., the action's commits page) and update the workflow to use the full SHA (format: owner/repo/path@<40-char-sha>) so the action refs are fully immutable.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/cifuzz.yml:
- Around line 1-40: The YAML workflow has inconsistent quoting and indentation
causing the formatting pipeline to fail; run the repository YAML/formatter and
normalize quotes and indenting for the whole file (ensure top-level keys like
"name: CIFuzz", the jobs block "jobs: fuzzing", and all steps such as "Build
Fuzzers (${{ matrix.sanitizer }})", "Run Fuzzers (${{ matrix.sanitizer }})" and
"Upload Sarif" use consistent indentation and quote style), then commit the
formatted .github workflow so CI passes.
---
Duplicate comments:
In @.github/workflows/cifuzz.yml:
- Line 21: Replace the short commit SHAs used in the workflow action pins with
full 40-character commit SHAs: locate the uses entry
"google/oss-fuzz/infra/cifuzz/actions/build_fuzzers@ba0e2e0" and the other two
uses: the entries flagged on lines 27 and 36, fetch the corresponding full
40-character commit SHA from each action's GitHub repository (e.g., the action's
commits page) and update the workflow to use the full SHA (format:
owner/repo/path@<40-char-sha>) so the action refs are fully immutable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 116d7031-c5b3-4424-823f-b4d29b37b532
📒 Files selected for processing (1)
.github/workflows/cifuzz.yml
…izer, SARIF always
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cifuzz.yml:
- Line 22: The checkout step using "uses:
actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" leaves the workflow
token persisted in git config; update that checkout step to set
persist-credentials: false so the token is not stored for subsequent steps
(modify the checkout step in the cifuzz workflow to include persist-credentials:
false alongside the existing uses line).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84a29c2e-1cfa-40ee-99f2-ee64bdd5b5b5
📒 Files selected for processing (1)
.github/workflows/cifuzz.yml
| matrix: | ||
| sanitizer: [address] | ||
| steps: | ||
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking checkout steps and credential persistence settings..."
nl -ba .github/workflows/cifuzz.yml | sed -n '18,32p'
echo
echo "Searching all workflows for checkout without persist-credentials:false..."
rg -n -C2 'uses:\s*actions/checkout@' .github/workflows
rg -n -C2 'persist-credentials:\s*false' .github/workflows || trueRepository: ory/hydra
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== cifuzz checkout step context ==="
sed -n '1,120p' .github/workflows/cifuzz.yml | sed -n '15,40p'
echo
echo "=== Find all checkout usages in workflows ==="
rg -n --hidden --glob '.github/workflows/**' 'uses:\s*actions/checkout@' .github/workflows
echo
echo "=== Find persist-credentials false usages ==="
rg -n --hidden --glob '.github/workflows/**' 'persist-credentials:\s*false' .github/workflows || true
echo
echo "=== Check whether cifuzz.yml checkout has with: persist-credentials ==="
python3 - <<'PY'
import re, pathlib
p = pathlib.Path(".github/workflows/cifuzz.yml")
txt = p.read_text()
# crude: locate checkout block and see if persist-credentials false appears within next ~20 lines
m = re.search(r'uses:\s*actions/checkout@[^\n]*', txt)
if not m:
print("No actions/checkout usage found in cifuzz.yml")
raise SystemExit(0)
start = m.start()
lines = txt[:start].count("\n")
block = txt.splitlines()
# locate line index of the checkout
idx = None
for i,l in enumerate(block):
if re.search(r'uses:\s*actions/checkout@', l):
idx = i; break
print(f"checkout line index (0-based): {idx}")
if idx is not None:
window = block[idx:idx+25]
has = any(re.search(r'persist-credentials:\s*false', l) for l in window)
print("persist-credentials:false present in next 25 lines:", has)
PYRepository: ory/hydra
Length of output: 1944
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path(".github/workflows/cifuzz.yml")
lines = p.read_text().splitlines()
for i in range(15,30):
l = lines[i-1]
# show leading whitespace counts
lead = len(l) - len(l.lstrip(' '))
print(f"{i:>3} lead={lead} |{l}|")
PYRepository: ory/hydra
Length of output: 743
Disable checkout credential persistence in cifuzz workflow
.github/workflows/cifuzz.yml uses actions/checkout without persist-credentials: false, which can leave the workflow token in git config for subsequent steps/actions.
🔧 Proposed hardening patch
- - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 22-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/cifuzz.yml at line 22, The checkout step using "uses:
actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" leaves the workflow
token persisted in git config; update that checkout step to set
persist-credentials: false so the token is not stored for subsequent steps
(modify the checkout step in the cifuzz workflow to include persist-credentials:
false alongside the existing uses line).
Source: Linters/SAST tools
2 participants
Adds Go native fuzz targets for OSS-Fuzz integration.
Criticality: 81/100 — Ory Hydra is an OpenID Certified OAuth2/OIDC provider used by enterprises worldwide. A token parsing bug = cross-organization authentication bypass.
Fuzz targets (6):
FuzzHMACTokenValidate— HMAC token validation with arbitrary attacker-controlled tokensFuzzHMACTokenSignature— Signature extraction used in token lookupFuzzHMACGenerateForString— Secret hashing with arbitrary inputsFuzzJWTValidate— JWT token validation (access tokens + ID tokens)FuzzJWTDecode— JWT header/payload/signature parsingFuzzJWTGetSignature— JWT signature extraction for revocationData sources:
🤖 Generated with Claude Code
Summary by CodeRabbit