Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Incorrect cycle detection logic in RedirectingClient #5491

Closed
trustin opened this issue Mar 8, 2024 · 2 comments · Fixed by #5548
Closed

Incorrect cycle detection logic in RedirectingClient #5491

trustin opened this issue Mar 8, 2024 · 2 comments · Fixed by #5548
Labels
defect sprint Issues for OSS Sprint participants

Comments

@trustin
Copy link
Member

trustin commented Mar 8, 2024

RedirectingClient uses LinkedListMultimap to record the list of visited URLs so far to detect and prevent cyclic redirects:

However, this doesn't have any effect because LinkedListMultimap.put() always returns true and isCyclicRedirect() mainly detects a cyclc by what LinkedListMultimap.put() returned:

We need to fix this by replacing LinkedListMultimap with a Set whose value is a tuple of:

  • HTTP method
  • whether the protocol is HTTP or HTTPS
    • h1, h2 and https should be considered equal.
    • h1c, h2c and http should be considered equal.
  • authority
  • path (including query)
@trustin trustin added the defect label Mar 8, 2024
@facewise
Copy link
Contributor

@trustin
Hello. Can I pick this up?

@trustin
Copy link
Member Author

trustin commented Mar 28, 2024

@facewise We're working on this internally unfortunately 😅

@moromin moromin mentioned this issue Mar 29, 2024
Merged
@jrhee17 jrhee17 added the sprint Issues for OSS Sprint participants label Apr 1, 2024
jrhee17 pushed a commit that referenced this issue Apr 8, 2024
@moromin
Improve incorrect cyclic redirect detection logic (#5548)
6a1ea7a 
Motivation:

Current implements can detect only cyclic redirect which comes back to
original URL like `a --> b --> c --> a`.
And cannot detect some cases, for example, partial cyclic one like `a
--> b --> c --> b` , however it returns
[TooManyRedirectsException](https://github.com/line/armeria/blob/db3973d74e70e177b0849f201fdd96615f2f6292/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java#L274-L277)
instead of
[CyclicRedirectsException](https://github.com/line/armeria/blob/db3973d74e70e177b0849f201fdd96615f2f6292/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java#L266-L270).

This is caused because adding redirect info to `Multimap` to check the
cyclic always returns `true`.
([code](https://github.com/line/armeria/blob/db3973d74e70e177b0849f201fdd96615f2f6292/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java#L587))
And I think the reasons we couldn't notice was that test cases were
insufficient and didn't validate the exception type.

To resolve the above problems, I add the following changes.

Modifications:

- Add `RedirectSignature` class to store redirect information.
- Use `Set` instead of `Multimap` to check cyclic redirections with the
above class.
    - `Set.add()` returns `false` when the same data is added.
- Add tests to verify some cases such as partial loop as mentioned in
motivation.
    - Verify `CyclicRedirectsException` is thrown.

Result:

- Closes #5491
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
defect sprint Issues for OSS Sprint participants
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve incorrect cyclic redirect detection logic moromin/armeria
3 participants
@trustin @jrhee17 @facewise