Skip to content

fix(pkg,gcp): logging level race, MaskToken short-input leak, registry-lock I/O, GetCredentials RPC #1054

Description

@cristim

Problem

A code-review pass over the shared pkg/ module and the GCP provider surfaced
several correctness bugs, two of them High severity, that none of the
in-flight PRs (#1012-#1035, #1036-#1049) touch. Grouping them here.

High

  • 10-H4 - Registry.GetProvider/GetAllProviders call the factory while
    holding the registry lock
    (pkg/provider/registry.go:60-105).
    The GCP factory does multi-page network I/O (getDefaultProject ->
    Projects.List().Pages(...)) when no project ID is configured. Running that
    under r.mu.RLock() blocks every other registry reader (and any writer, e.g.
    Unregister) for the duration of an arbitrary user-supplied callback that
    performs network calls. GetAllProviders additionally serializes N providers'
    network init under one lock.
    Fix: snapshot the factory (and name) under the lock, release it, then call the
    factory outside the critical section.

  • 10-H3 - gcp.GetCredentials issues a live GetProject RPC
    (providers/gcp/provider.go:270-293).
    GetCredentials gates on IsConfigured(), which constructs a
    resourcemanager client and performs a GetProject round-trip purely to
    report the credential source. DetectAvailableProviders already calls
    IsConfigured() + ValidateCredentials(), so a follow-on GetCredentials()
    triggers a third GetProject. Credential introspection (which source) is
    conflated with validation (do they work).
    Fix: report the source from env/ADC inspection without a network call.

Low / Nit

  • 10-L5 - pkg/logging level read/write data race
    (pkg/logging/logger.go). defaultLogger.level is read in every
    Debug/Info/... and written by SetLevel/SetLevelValue with no
    synchronization -> a real -race data race if SetLevel is called after
    goroutines launch (the concurrent fan-out logs from many goroutines).
    Fix: store the level in an atomic.Int32.

  • 10-L6 - MaskToken leaks short inputs verbatim
    (pkg/common/tokens.go). The function returns inputs of <=8 chars unchanged.
    Current callers only pass 64-char hex tokens, but the generic "return
    unchanged" branch is a footgun for any future short secret.
    Fix: redact short non-empty inputs to (redacted) instead of echoing them.

  • 10-N4 - errors.Is matches purely on type
    (pkg/errors/errors.go). errors.Is(specificNotFound, &NotFoundError{ID:"x"})
    returns true regardless of struct fields. This is the documented intent
    (type-level sentinel matching), but the field-insensitivity is undocumented.
    Fix: document the type-level matching contract on each Is method.

  • 06-N2 - RedactedDSN duplicates the DSN format string
    (internal/database/config.go:171). The redacted variant copies the host=... port=... user=... format from DSN; a future field change must be made in two
    places. Fix: derive the redacted form from a single source.

Fix

See per-finding fixes above. Each correctness fix gets a regression test
(notably the logging-level data race under -race and the MaskToken
short-input case).

Files

  • pkg/provider/registry.go
  • providers/gcp/provider.go
  • pkg/logging/logger.go
  • pkg/common/tokens.go
  • pkg/errors/errors.go
  • internal/database/config.go

Findings

10-H3, 10-H4, 10-L5, 10-L6, 10-N4, 06-N2.

Activity

  1. added
    triagedItem has been triaged
    pr-createdA PR has been opened for this issue (dedup guard for the auto-PR loop)
    pr-mergedThe PR for this issue has been merged
    on Jun 7, 2026
  2. cristim commented on Jul 27, 2026

    @cristim
    MemberAuthor

    Verification sweep against main (101f099fb) on 2026-07-27 finds this already resolved.
    All six items are fixed: atomic logging level, registry lock, GetCredentials without RPC, MaskToken short-input handling, errors doc, and DSN dedup.
    Evidence: commit 43a009c (PR #1076).
    Recommending close. Reopen if any of the six sub-items regressed.

  3. cristim commented on Aug 24, 2026

    @cristim
    MemberAuthor

    Already implemented on main. Closing with evidence rather than scheduling work.

    Verified against origin/main (ae1e632cb), spot-checking the findings this issue names:

    • Logging level race: pkg/logging/logger.go:37 stores level atomic.Int32, with the comment at :32 recording that SetLevel/SetLevelValue are called at runtime concurrently with reads.
    • MaskToken short-input leak: pkg/common/tokens.go:78 returns (none) for empty and (redacted) for len(token) <= 8, only truncating at token[:8] + "..." above that. A short token can no longer be echoed whole.
    • RedactedDSN drift: internal/database/config.go:166 is return c.dsn("*****"), deriving from the same builder as the real DSN rather than re-implementing the layout, so the two cannot diverge. TestRedactedDSN_SharesLayoutWithDSN pins exactly that.

    The registry-lock finding (factories snapshotted outside the lock, GetCredentials doing zero RPC) and the errors.Is documentation are addressed in the same change, 43a009c7e, which carried no closing trailer.

    If a residual axis remains that this does not cover, please reopen naming it specifically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort/mDaysimpact/internalTeam-internal onlypr-createdA PR has been opened for this issue (dedup guard for the auto-PR loop)pr-mergedThe PR for this issue has been mergedpriority/p1Next up; this sprintseverity/highSignificant harmtriagedItem has been triagedtype/bugDefecturgency/this-sprintWithin the current sprint

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions