Skip to content

Do a pass hardening the authentication module - #1235

Merged
jviotti merged 2 commits into
mainfrom
fix-auth
Aug 18, 2026
Merged

Do a pass hardening the authentication module#1235
jviotti merged 2 commits into
mainfrom
fix-auth

Conversation

@jviotti

@jviotti jviotti commented Aug 18, 2026

Copy link
Copy Markdown
Member

Signed-off-by: Juan Cruz Viotti jv@jviotti.com

Review in cubic

@augmentcode

augmentcode Bot commented Aug 18, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR hardens Enterprise authentication artifact handling and silent OIDC renewal.

Changes:

  • Replaces plaintext renewal-policy cookies with signed, purpose-bound renewal markers.
  • Restricts marker acceptance to the interactive policy governing the requested path.
  • Adds a renewal-specific sealing purpose and end-to-end coverage for valid, forged, and cross-scope markers.
  • Rejects authentication artifacts whose node masks reference nonexistent policies.
  • Writes compiled authentication tables atomically to avoid mutating bytes beneath active mappings.
  • Distinguishes unreadable authentication tables from paths governed by no policy.
  • Uses that distinction conservatively in explorer privacy and protected-resource metadata generation.
  • Rejects configuration aliases between caller-supplied authentication keys and server-side secrets.
  • Prevents reference traversal from policies whose audience resolves to nobody.
  • Captures request log text before asynchronous response handling can release the request.
Technical notes: Static assets are intended to receive private caching when gated; the review identifies an issue in that new selection path.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/actions/action_serve_static_v1.h Outdated
resolution.path.value(), sourcemeta::core::HTTP_STATUS_OK, false, {},
{}, {}, request, response, this->error_schema_,
"public, max-age=31536000, immutable", "Accept-Encoding");
resolution.is_public ? "public, max-age=31536000, immutable"

@augmentcode augmentcode Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/actions/action_serve_static_v1.h:100: artifact_resolve_static returns the default ArtifactResolution::is_public == true and receives no caller or authorization context, so this branch always selects public even when the route is policy-gated. A shared cache may then retain and serve an authenticated static response without rechecking the route gate.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 23 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="enterprise/authentication/authentication_save.cc">

<violation number="1" location="enterprise/authentication/authentication_save.cc:463">
P1: On Windows, rebuilding while the server is running now fails when publishing `authentication.bin`. `FileView` does not grant delete sharing, so the atomic rename cannot replace the mapped destination; allow delete sharing for this mapping or use a Windows replacement strategy that supports open readers.</violation>

<violation number="2" location="enterprise/authentication/authentication_save.cc:463">
P2: When an existing `authentication.bin` has restrictive permissions or ACLs that the server relies on, this replacement discards them and installs the staging file's defaults. Preserve the destination metadata or apply an explicit secure mode and ownership to the replacement.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

// rewriting it in place changes bytes underneath a reader that has already
// proved them sound. Replacing it as a whole leaves that reader on the file
// it validated until it is restarted
sourcemeta::core::atomic_write_file(destination, bytes);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: On Windows, rebuilding while the server is running now fails when publishing authentication.bin. FileView does not grant delete sharing, so the atomic rename cannot replace the mapped destination; allow delete sharing for this mapping or use a Windows replacement strategy that supports open readers.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/authentication/authentication_save.cc, line 463:

<comment>On Windows, rebuilding while the server is running now fails when publishing `authentication.bin`. `FileView` does not grant delete sharing, so the atomic rename cannot replace the mapped destination; allow delete sharing for this mapping or use a Windows replacement strategy that supports open readers.</comment>

<file context>
@@ -456,7 +456,11 @@ auto Authentication::Table::compile(
+  // rewriting it in place changes bytes underneath a reader that has already
+  // proved them sound. Replacing it as a whole leaves that reader on the file
+  // it validated until it is restarted
+  sourcemeta::core::atomic_write_file(destination, bytes);
 }
 
</file context>

Comment thread src/actions/action_serve_static_v1.h Outdated
// rewriting it in place changes bytes underneath a reader that has already
// proved them sound. Replacing it as a whole leaves that reader on the file
// it validated until it is restarted
sourcemeta::core::atomic_write_file(destination, bytes);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When an existing authentication.bin has restrictive permissions or ACLs that the server relies on, this replacement discards them and installs the staging file's defaults. Preserve the destination metadata or apply an explicit secure mode and ownership to the replacement.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/authentication/authentication_save.cc, line 463:

<comment>When an existing `authentication.bin` has restrictive permissions or ACLs that the server relies on, this replacement discards them and installs the staging file's defaults. Preserve the destination metadata or apply an explicit secure mode and ownership to the replacement.</comment>

<file context>
@@ -456,7 +456,11 @@ auto Authentication::Table::compile(
+  // rewriting it in place changes bytes underneath a reader that has already
+  // proved them sound. Replacing it as a whole leaves that reader on the file
+  // it validated until it is restarted
+  sourcemeta::core::atomic_write_file(destination, bytes);
 }
 
</file context>

Comment thread enterprise/authentication/authentication_table.h Outdated
Comment thread test/cli/index/enterprise/snapshot-authentication-shared-provider-secrets.sh Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark Index (community)

Details
Benchmark suite Current: fc93001 Previous: a0bcde5 Ratio
Add one schema (0 existing) 319 ms 337 ms 0.95
Add one schema (100 existing) 40 ms 46 ms 0.87
Add one schema (1000 existing) 99 ms 92 ms 1.08
Add one schema (10000 existing) 832 ms 1019 ms 0.82
Update one schema (1 existing) 30 ms 35 ms 0.86
Update one schema (101 existing) 38 ms 38 ms 1
Update one schema (1001 existing) 99 ms 94 ms 1.05
Update one schema (10001 existing) 801 ms 988 ms 0.81
Cached rebuild (1 existing) 11 ms 10 ms 1.10
Cached rebuild (101 existing) 15 ms 15 ms 1
Cached rebuild (1001 existing) 42 ms 39 ms 1.08
Cached rebuild (10001 existing) 342 ms 337 ms 1.01
Index 100 schemas 576 ms 625 ms 0.92
Index 1000 schemas 1424 ms 1455 ms 0.98
Index 10000 schemas 12468 ms 12980 ms 0.96
Index 10000 schemas (custom meta-schema) 14430 ms 15375 ms 0.94
Index 10000 schemas ($ref fan-out) 15600 ms 15429 ms 1.01

This comment was automatically generated by workflow using github-action-benchmark.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark Index (enterprise)

Details
Benchmark suite Current: fc93001 Previous: a0bcde5 Ratio
Add one schema (0 existing) 381 ms 383 ms 0.99
Add one schema (100 existing) 117 ms 118 ms 0.99
Add one schema (1000 existing) 166 ms 170 ms 0.98
Add one schema (10000 existing) 750 ms 800 ms 0.94
Update one schema (1 existing) 106 ms 111 ms 0.95
Update one schema (101 existing) 115 ms 115 ms 1
Update one schema (1001 existing) 169 ms 170 ms 0.99
Update one schema (10001 existing) 772 ms 809 ms 0.95
Cached rebuild (1 existing) 13 ms 10 ms 1.30
Cached rebuild (101 existing) 14 ms 16 ms 0.88
Cached rebuild (1001 existing) 41 ms 41 ms 1
Cached rebuild (10001 existing) 306 ms 316 ms 0.97
Index 100 schemas 581 ms 559 ms 1.04
Index 1000 schemas 1569 ms 1573 ms 1.00
Index 10000 schemas 13157 ms 13388 ms 0.98
Index 10000 schemas (custom meta-schema) 15552 ms 16292 ms 0.95
Index 10000 schemas ($ref fan-out) 15842 ms 16477 ms 0.96

This comment was automatically generated by workflow using github-action-benchmark.

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@jviotti
jviotti merged commit 5b2b37f into main Aug 18, 2026
6 checks passed
@jviotti
jviotti deleted the fix-auth branch August 18, 2026 13:33
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.

1 participant