fix(security): secure OAuth callback redirects - #3984
Conversation
Co-authored-by: Codex GPT-5 <codex@openai.com>
awildturtok
left a comment
awildturtok
left a comment
There was a problem hiding this comment.
sieht gut aus aber muss es mir nochmal mit klarerem kopf anschauen
|
|
||
| @JsonIgnore | ||
| @Getter(AccessLevel.NONE) | ||
| private final Cache<String, PendingAuthorizationRequest> pendingAuthorizationRequests = Caffeine.newBuilder() |
There was a problem hiding this comment.
=> CaffeinSpec, und 10K wirkt absurd viel.
There was a problem hiding this comment.
Es sind absurd viele Einträge, aber aber der footprint ist niedrig und wichtig ist dass wir zwar bounded sind, aber nie gegen das limit laufen werden.
| } | ||
|
|
||
| String registerAuthorizationRequest(URI callbackUri, URI returnUri) { | ||
| final String state = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
ist "state" ein oauth name? Meiner intuition nach ist das ja eigentlich ein Identifier?
There was a problem hiding this comment.
Das ist der State der hier gemeint ist, und für so einen Use-Case gedacht: https://www.rfc-editor.org/info/rfc6749/#section-4.1.1
There was a problem hiding this comment.
ok, das ist dann ein doof gewählter name in der spec. Danke
| } | ||
|
|
||
| static URI toRootRelativeUri(URI requestUri) { | ||
| if (requestUri == null || !requestUri.isAbsolute()) { |
There was a problem hiding this comment.
der null check sollte separat sein imo?
| final JwtPkceVerifyingRealmFactory factory = new JwtPkceVerifyingRealmFactory(); | ||
| final URI callbackUri = URI.create("https://example.com/admin-ui"); | ||
| final String state = factory.registerAuthorizationRequest(callbackUri, URI.create("/admin-ui/users")); | ||
| factory.validateAndConsumeAuthorizationRequest(state, callbackUri); |
There was a problem hiding this comment.
bitte hier kommentar dran machen, dass das stateful ist und ab hier nicht reused werden kann
There was a problem hiding this comment.
Habs an der Methode kommentiert und auf das "consume" hingedeutet, da die wird hier mehrere male benutzt im test
| ); | ||
| assertThrows( | ||
| BadRequestException.class, | ||
| () -> JwtPkceVerifyingRealmFactory.toRootRelativeUri(URI.create("https://example.com//attacker.example/path")) |
There was a problem hiding this comment.
Ist ein Test gegen path-traversal/-filtering Attacken.
2 participants
No description provided.