Skip to content

fix(portable): treat an empty OCI credential as absent instead of sending an empty bearer token - #1343

Merged
DecisionNerd merged 1 commit into
mainfrom
fix/1336-oci-credential
Sep 16, 2026
Merged

DecisionNerd merged 1 commit into
mainfrom
fix/1336-oci-credential

Conversation

@DecisionNerd

@DecisionNerd DecisionNerd commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Defect

oci_credential() in crates/graphforge-cli/src/portable_cli.rs was
std::env::var("GRAPHFORGE_OCI_CREDENTIAL").ok(). A variable that is exported
but unset — the ordinary GRAPHFORGE_OCI_CREDENTIAL="${REGISTRY_TOKEN}" shell
and CI pattern — yields Ok(""), so the function returned Some("") rather
than None. Nothing downstream filtered it: HttpOciRegistry::new validated
only for \n and \r, and auth_header routes any colon-free token to the
Bearer arm. The result was the literal header value Bearer with an empty
token on all six OCI request paths, so an anonymous pull from a public registry
that should send no header was rejected, and the failure surfaced as an
authentication error instead of a missing credential.

crates/graphforge-search/src/provider_openrouter.rs already treats an empty
bearer credential as absent; the OCI path was inconsistent with that precedent.

Secondary finding: HttpOciRegistry::new rejected only CR and LF while
reporting "credential contains control characters".

Fix

  • oci_credential now takes the raw value and maps empty or whitespace-only
    input to None. Taking the value as an argument keeps the mapping a pure
    function, so it is tested directly rather than by mutating process-global
    environment state (std::env::set_var is unsafe and racy under parallel test
    execution). The env read moves to the two call sites behind a named
    OCI_CREDENTIAL_ENV constant.
  • HttpOciRegistry::new applies the same normalization at the transport
    boundary, so a direct API caller cannot produce an empty bearer token either.
  • The control-character check is widened from \n/\r to all control
    characters, so the check and its message agree. This strengthens rather than
    weakens the refusal: CR and LF are still rejected, header injection is still
    blocked, and the structured error class stays PortableV2ErrorCode::InvalidPath.

Basic/Bearer selection for non-empty credentials is unchanged, and present
credential values pass through untrimmed.

Mutation score

Scoped command from the issue:

cargo mutants --file crates/graphforge-cli/src/portable_cli.rs --package graphforge-cli -- --tests
mutants caught missed unviable
before 22 14 3 5
after 23 18 0 5

The three prior survivors were all at oci_credential — replacing it with
None, with Some(String::new()), and with Some("xyzzy".into()). All three
are now caught, along with the new delete ! mutant in the same function. The
five unviable mutants are unchanged and unrelated (Default::default() on types
that do not implement Default).

Verification

  • New unit tests in crates/graphforge-cli/src/portable_cli.rs cover absent,
    empty, whitespace-only, plain-token and colon-bearing inputs, plus the env
    variable name, without touching process-global state.
  • New tests in crates/graphforge-portable-oci/src/tests.rs assert the
    credential-to-header mapping across the same five input classes, prove over a
    real loopback HTTP server that an empty credential sends no Authorization
    header at all while the pull still succeeds, and prove that CR, LF, TAB and
    other control characters are still refused with the matching message and error
    class while an ordinary token is accepted.
  • No credential value is printed by any test; assertions compare values rather
    than logging them, and the existing "errors never contain the credential"
    tests still pass.
  • cargo test -p graphforge-cli -p graphforge-portable-oci --lib,
    cargo clippy -p graphforge-cli -p graphforge-portable-oci --all-targets
    (no new diagnostics), cargo fmt --all -- --check, and make pre-push-fast
    all pass locally. No new test files were added, so no Bazel target, migration
    ledger, or parity changes were required; bazel-migration-ledger-check.py,
    cargo-bazel-parity-check.py --mode inventory and
    cargo-bazel-drift-check.py were run and pass.

Closes #1336

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@coderabbitai

coderabbitai Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 10db0432-4ef8-435b-8101-d9a8f7a14df4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

`oci_credential()` returned `Some("")` for a `GRAPHFORGE_OCI_CREDENTIAL`
that is exported but unset, because `std::env::var` yields `Ok("")` for it.
Nothing filtered that value, and `auth_header` routes any colon-free token
to the Bearer arm, so every OCI request carried the literal header
`Bearer ` with an empty token. An anonymous pull that should send no header
was rejected as an authentication failure, pointing the operator at the
wrong cause.

`oci_credential` now takes the raw value and maps empty or whitespace-only
input to `None`, matching the position `provider_openrouter` already takes.
Taking the value as an argument keeps the mapping a pure function, so it is
tested directly instead of through process-global environment mutation.
`HttpOciRegistry::new` applies the same normalization at the transport
boundary, and its control-character check is widened from CR and LF to all
control characters so the refusal matches the message it already reported.
The CR and LF refusal that blocks header injection is unchanged, and the
structured error class stays `InvalidPath`.

Scoped mutation score for `crates/graphforge-cli/src/portable_cli.rs`:
22 mutants, 14 caught, 3 missed, 5 unviable before; 23 mutants, 18 caught,
0 missed, 5 unviable after. All three former survivors were at
`oci_credential`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@DecisionNerd
DecisionNerd force-pushed the fix/1336-oci-credential branch from 9efac10 to a3751a0 Compare September 16, 2026 20:40
@github-actions github-actions Bot added the core Core source code changes label Sep 16, 2026
@DecisionNerd
DecisionNerd added this pull request to the merge queue Sep 16, 2026
Merged via the queue into main with commit 119e229 Sep 16, 2026
21 checks passed
@DecisionNerd
DecisionNerd deleted the fix/1336-oci-credential branch September 17, 2026 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core source code changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(portable): treat an empty OCI credential as absent instead of sending an empty bearer token

1 participant