Skip to content

fix: correct audit log spread ordering to prevent org id being set undefined#5751

Merged
scott-ray-wilson merged 1 commit intomainfrom
fix-secret-sharing-audit-ordering
Mar 18, 2026
Merged

fix: correct audit log spread ordering to prevent org id being set undefined#5751
scott-ray-wilson merged 1 commit intomainfrom
fix-secret-sharing-audit-ordering

Conversation

@scott-ray-wilson
Copy link
Copy Markdown
Contributor

Context

This PR fixes a bug where sharing a secret from an org to a non-org user was encountering an error due to the spread order overwriting the audit log org Id

Screenshots

N/A

Steps to verify the change

  • BEFORE PULLING PR - share a secret from an org, open the link in incognito, see error
  • pull changes, refresh, no error

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Checklist

  • Title follows the conventional commit format: type(scope): short description (scope is optional, e.g., fix: prevent crash on sync or fix(api): handle null response).
  • Tested locally
  • Updated docs (if needed)
  • Updated CLAUDE.md files (if needed)
  • Read the contributing guide

@maidul98
Copy link
Copy Markdown
Collaborator

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 18, 2026

Greptile Summary

This PR fixes a one-line bug in the POST /:id/access (access shared secret) route where the spread order of the createAuditLog call caused orgId: sharedSecret.orgId to be silently overwritten by ...req.auditLogInfo — which carries orgId: undefined for unauthenticated (non-org) users — resulting in an error when a shared secret scoped to an org was opened in an anonymous session.

  • The core fix is correct: placing orgId: sharedSecret.orgId after ...req.auditLogInfo ensures the explicit value wins the spread merge.
  • Two other createAuditLog calls in the same file (CREATE_SHARED_SECRET at line 299 and DELETE_SHARED_SECRET at line 350) still use the pre-fix ordering (orgId before the spread). These are not currently buggy because those routes require authentication, but they are inconsistent with the fixed pattern and could mask future regressions.

Confidence Score: 4/5

  • Safe to merge — the fix is minimal, correct, and well-scoped to the reported bug.
  • The change is a single two-line reorder that directly addresses the root cause (spread overwriting an explicit property). No logic is added or removed. The only concern is that two sibling audit log calls in the same file retain the old ordering, but they do not trigger the bug today since their routes require authentication.
  • Attention should be paid to the createSharedSecret and deleteSharedSecret audit log calls (lines 299 and 350) for ordering consistency.

Important Files Changed

Filename Overview
backend/src/server/routes/v1/secret-sharing-router.ts Fixes spread ordering in the accessSharedSecret audit log call so orgId: sharedSecret.orgId is not overwritten by ...req.auditLogInfo; two other audit log calls in this file (CREATE and DELETE routes) still use the old ordering pattern, though they are not buggy in practice since those routes require authentication.

Comments Outside Diff (1)

  1. backend/src/server/routes/v1/secret-sharing-router.ts, line 299-301 (link)

    P2 Inconsistent spread ordering — same root cause as the fixed bug

    The CREATE_SHARED_SECRET (line 299) and DELETE_SHARED_SECRET (line 350) audit log calls still put orgId before ...req.auditLogInfo, which is the same ordering that this PR is fixing in the /access route.

    In practice this doesn't cause a bug today because both routes are protected by verifyAuth, so req.auditLogInfo.orgId will match req.permission.orgId. However, for consistency and to guard against future regressions, the safer pattern used in the fix (explicit orgId placed after the spread) should be applied everywhere in this file.

    The same change applies at line 350:

          await server.services.auditLog.createAuditLog({
            ...req.auditLogInfo,
            orgId: req.permission.orgId,
    

Last reviewed commit: "fix: correct audit l..."

@scott-ray-wilson scott-ray-wilson merged commit 0161e21 into main Mar 18, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants