Skip to content

Correctly fallback to OIDC nested policy paths#1137

Merged
jviotti merged 3 commits into
mainfrom
redirect-fallback
Jul 17, 2026
Merged

Correctly fallback to OIDC nested policy paths#1137
jviotti merged 3 commits into
mainfrom
redirect-fallback

Conversation

@jviotti

@jviotti jviotti commented Jul 17, 2026

Copy link
Copy Markdown
Member

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

Review in cubic

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

augmentcode Bot commented Jul 17, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR makes OIDC logins fall back to the first path governed by the matched policy when no valid to return target is provided.

Changes:

  • Adds default_path to serialized OIDC policy metadata and to Authentication::InteractivePolicy
  • Bumps the authentication artifact format version to 9
  • Updates the OIDC login action to seal a policy-based fallback redirect target
  • Adjusts e2e Hurl expectations and adds unit coverage for default-path selection

🤖 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. 2 suggestions posted.

Fix All in Augment

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

@@ -81,14 +81,16 @@ auto encode_oidc_metadata(const std::string_view issuer,
const std::string_view client_id,

@augmentcode augmentcode Bot Jul 17, 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.

enterprise/authentication/authentication_save.cc:81 The comment immediately above encode_oidc_metadata describes the encoded fields but doesn’t mention the newly-serialized default_path, so the documented metadata layout is now incomplete.

Severity: low

Fix This in Augment

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

// taken as-is, and only a same-origin local path is honoured, so the login
// cannot be turned into an open redirect. Anything else falls back to what
// the policy governs. It is sealed into the transaction so that the
// callback trusts it

@augmentcode augmentcode Bot Jul 17, 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.

enterprise/server/include/sourcemeta/one/enterprise_server_action_auth_login_v1.h:141 This comment says the callback “trusts” the sealed to, but the callback action still re-validates it with is_local_path before redirecting; the wording may mislead readers about the defense-in-depth behavior.

Severity: low

Fix This in Augment

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

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

@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.

All reported issues were addressed across 10 files

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

Re-trigger cubic

Comment thread enterprise/authentication/authentication_format.h
Comment thread enterprise/server/include/sourcemeta/one/enterprise_server_action_auth_login_v1.h 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: 438959b Previous: 37bc023 Ratio
Add one schema (0 existing) 327 ms 303 ms 1.08
Add one schema (100 existing) 28 ms 30 ms 0.93
Add one schema (1000 existing) 86 ms 95 ms 0.91
Add one schema (10000 existing) 732 ms 791 ms 0.93
Update one schema (1 existing) 21 ms 21 ms 1
Update one schema (101 existing) 28 ms 31 ms 0.90
Update one schema (1001 existing) 90 ms 95 ms 0.95
Update one schema (10001 existing) 735 ms 792 ms 0.93
Cached rebuild (1 existing) 7 ms 8 ms 0.88
Cached rebuild (101 existing) 9 ms 10 ms 0.90
Cached rebuild (1001 existing) 30 ms 35 ms 0.86
Cached rebuild (10001 existing) 258 ms 288 ms 0.90
Index 100 schemas 649 ms 618 ms 1.05
Index 1000 schemas 1325 ms 1303 ms 1.02
Index 10000 schemas 13840 ms 13420 ms 1.03
Index 10000 schemas (custom meta-schema) 16717 ms 15970 ms 1.05
Index 10000 schemas ($ref fan-out) 17003 ms 16138 ms 1.05

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: 438959b Previous: 37bc023 Ratio
Add one schema (0 existing) 402 ms 401 ms 1.00
Add one schema (100 existing) 115 ms 105 ms 1.10
Add one schema (1000 existing) 178 ms 154 ms 1.16
Add one schema (10000 existing) 918 ms 746 ms 1.23
Update one schema (1 existing) 107 ms 95 ms 1.13
Update one schema (101 existing) 112 ms 100 ms 1.12
Update one schema (1001 existing) 177 ms 154 ms 1.15
Update one schema (10001 existing) 879 ms 745 ms 1.18
Cached rebuild (1 existing) 9 ms 8 ms 1.13
Cached rebuild (101 existing) 12 ms 10 ms 1.20
Cached rebuild (1001 existing) 37 ms 30 ms 1.23
Cached rebuild (10001 existing) 295 ms 251 ms 1.18
Index 100 schemas 493 ms 761 ms 0.65
Index 1000 schemas 1570 ms 1450 ms 1.08
Index 10000 schemas 14068 ms 13553 ms 1.04
Index 10000 schemas (custom meta-schema) 17140 ms 16366 ms 1.05
Index 10000 schemas ($ref fan-out) 17251 ms 16391 ms 1.05

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

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@jviotti
jviotti merged commit bc928cc into main Jul 17, 2026
5 checks passed
@jviotti
jviotti deleted the redirect-fallback branch July 17, 2026 14:02
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