Improve incorrect cyclic redirect detection logic #5548

moromin · 2024-03-29T15:44:11Z

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 instead of CyclicRedirectsException.

This is caused because adding redirect info to Multimap to check the cyclic always returns true. (code)
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 Incorrect cycle detection logic in RedirectingClient #5491

CLAassistant · 2024-03-29T15:44:18Z

All committers have signed the CLA.

github-actions · 2024-03-29T16:03:41Z

🔍 Build Scan® (commit: `68c60ca`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/dft53py4ihznq
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/x3zrnd6xklvw4
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/m3jepq6sasr2q
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/noubw4kulxdqy
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/2m4b3ovogka5i
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/upzuvt3kxbpe2
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/ci5h7uy6noyby
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/utmnq4qtgpaym

codecov · 2024-03-29T16:36:48Z

Codecov Report

Attention: Patch coverage is 92.85714% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 74.11%. Comparing base (db3973d) to head (8627b5c).
Report is 22 commits behind head on main.

❗ Current head 8627b5c differs from pull request most recent head 68c60ca. Consider uploading reports for the commit 68c60ca to get more accurate results

Files	Patch %	Lines
...com/linecorp/armeria/client/RedirectingClient.java	92.85%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5548      +/-   ##
============================================
+ Coverage     74.09%   74.11%   +0.02%     
- Complexity    20907    20974      +67     
============================================
  Files          1809     1818       +9     
  Lines         76940    77259     +319     
  Branches       9833     9860      +27     
============================================
+ Hits          57005    57259     +254     
- Misses        15291    15321      +30     
- Partials       4644     4679      +35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

trustin · 2024-04-01T07:11:51Z

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java

+            // Minus 1 because the original signature is also included.
+            if (redirectSignatures.size() - 1 > maxRedirects) {


+1 for -1 😆

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java

trustin

We're getting there :-) Nice!

trustin · 2024-04-02T13:16:29Z

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java

-            if (redirectUris.size() > maxRedirects) {
+            final Set<RedirectSignature> redirectSignatures = redirectCtx.redirectSignatures();
+            // Plus 1 to maxRedirects because the original signature is also included.
+            if (redirectSignatures.size() > maxRedirects + 1) {


I'd recommend using -1 to avoid potential overflow. Imagine a user specifies Integer.MAX_VALUE for maxRedirects.

trustin · 2024-04-02T13:19:15Z

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java

+            return protocol.equals(that.protocol) &&
+                   authority.equals(that.authority) &&
+                   pathAndQuery.equals(that.pathAndQuery) &&
+                   method == that.method;


How about ordering the comparison in more cost-effective way? For example, we could compare pathAndQuery and authority before others because they are more likely to change, although it may not always be the case.

trustin · 2024-04-02T13:19:37Z

core/src/test/java/com/linecorp/armeria/client/RedirectSignatureTest.java

+import com.linecorp.armeria.client.RedirectingClient.RedirectSignature;
+import com.linecorp.armeria.common.HttpMethod;
+
+public class RedirectSignatureTest {


public is redundant in JUnit 5.

Suggested change

public class RedirectSignatureTest {

class RedirectSignatureTest {

trustin · 2024-04-02T13:20:10Z

core/src/test/java/com/linecorp/armeria/client/RedirectSignatureTest.java

+            new RedirectSignature(PROTOCOL, AUTHORITY, PATH_AND_QUERY, METHOD);
+
+    @Test
+    public void equalityWithSameObject() {


public is redundant in JUnit 5 test methods. Could you remove all public keywords where applicable?

…dundant parts

trustin

ikhoon · 2024-04-03T06:21:11Z

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java

+            if (redirectSignatures == null) {
+                redirectSignatures = new LinkedHashSet<>();
+            }
+
+            if (!isAddedOriginalSignature) {
+                final String originalProtocol = ctx.sessionProtocol().isTls() ? "https" : "http";
+                final RedirectSignature originalSignature = new RedirectSignature(originalProtocol,
+                                                                                  ctx.authority(),
+                                                                                  request.headers().path(),
+                                                                                  request.method());
+                redirectSignatures.add(originalSignature);
+                isAddedOriginalSignature = true;
+            }


Question) Could we add originalSignature as redirectSignatures is allocated?
For example:

Suggested change

if (redirectSignatures == null) {

redirectSignatures = new LinkedHashSet<>();

}

if (!isAddedOriginalSignature) {

final String originalProtocol = ctx.sessionProtocol().isTls() ? "https" : "http";

final RedirectSignature originalSignature = new RedirectSignature(originalProtocol,

ctx.authority(),

request.headers().path(),

request.method());

redirectSignatures.add(originalSignature);

isAddedOriginalSignature = true;

}

if (redirectSignatures == null) {

redirectSignatures = new LinkedHashSet<>();

final String originalProtocol = ctx.sessionProtocol().isTls() ? "https" : "http";

final RedirectSignature originalSignature = new RedirectSignature(originalProtocol,

ctx.authority(),

request.headers().path(),

request.method());

redirectSignatures.add(originalSignature);

}

yes, I will also remove isAddedOriginalSignature flag.

ikhoon · 2024-04-03T06:41:34Z

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java

-            // Always called after addRedirectUri is called.
-            assert redirectUris != null;
-            return redirectUris;
+        public String uri() {


Suggested change

public String uri() {

String uri() {

ikhoon · 2024-04-03T06:46:26Z

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java

+            final boolean isCyclicRedirects =
+                    !redirectCtx.addRedirectSignature(nextReqTarget,
+                                                      newReqDuplicator.headers().method());
+            if (isCyclicRedirects) {
                handleException(ctx, derivedCtx, reqDuplicator, responseFuture, response,
                                CyclicRedirectsException.of(redirectCtx.originalUri(),


Optional) What do you think of raising either CyclicRedirectsException or TooManyRedirectsException by addRedirectSignature() which may streamline the code?

I will try it 👍

jrhee17

Looks good to me 👍 Thanks @moromin ! 🙇 👍 🙇

minwoox

Looks great! Thanks a lot! 😄

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java

ikhoon

Looks solid. Thanks, @moromin! 🙇‍♂️👍

moromin added 2 commits March 29, 2024 19:39

feat: replace Multimap with Set to detect cyclic redirects correctly

8ba86c4

test: add test cases for protocol, authority, query

1874502

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin March 29, 2024 15:44

style: fix style errors

96216a2

moromin added 4 commits March 31, 2024 23:37

test: add tests for RedirectSignature

f53a05a

test: add tests to verify equality

01eab00

refactor: wrap client factory with try to free resources

a4b6555

test: try to trigger ci again

40b949c

trustin reviewed Apr 1, 2024

View reviewed changes

moromin added 4 commits April 1, 2024 17:33

refactor: make the method inline and the set just hash set

eb394c4

style: remove unused import

731518f

refactor: change signature fields to avoid string manipulation

6436db1

refactor: modify variable name

bd1a3aa

trustin reviewed Apr 2, 2024

View reviewed changes

trustin added defect sprint Issues for OSS Sprint participants labels Apr 2, 2024

refactor: make comparing equality method more effective and remove re…

4235b73

…dundant parts

trustin added this to the 1.28.0 milestone Apr 3, 2024

trustin approved these changes Apr 3, 2024

View reviewed changes

ikhoon reviewed Apr 3, 2024

View reviewed changes

moromin added 4 commits April 3, 2024 16:43

refactor: add origin when signatures is allocated

41d018b

refactor: remove redundant public

a13405c

refactor: validate cyclic redirects in context and throw exception

8b03a3f

test: trigger ci test

8627b5c

jrhee17 approved these changes Apr 4, 2024

View reviewed changes

minwoox approved these changes Apr 4, 2024

View reviewed changes

ikhoon reviewed Apr 8, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java Outdated Show resolved Hide resolved

moromin added 2 commits April 8, 2024 15:17

refactor: handle TooManyRedirectsException in redirectCtx

b80a42e

test: trigger ci test

68c60ca

ikhoon approved these changes Apr 8, 2024

View reviewed changes

jrhee17 merged commit 6a1ea7a into line:main Apr 8, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve incorrect cyclic redirect detection logic #5548

Improve incorrect cyclic redirect detection logic #5548

moromin commented Mar 29, 2024

CLAassistant commented Mar 29, 2024 •

edited

github-actions bot commented Mar 29, 2024 •

edited

codecov bot commented Mar 29, 2024 •

edited

trustin Apr 1, 2024

trustin left a comment

trustin Apr 2, 2024

trustin Apr 2, 2024

trustin Apr 2, 2024

trustin Apr 2, 2024

trustin left a comment

ikhoon Apr 3, 2024

moromin Apr 3, 2024

ikhoon Apr 3, 2024

ikhoon Apr 3, 2024

moromin Apr 3, 2024 •

edited

jrhee17 left a comment

minwoox left a comment

ikhoon left a comment

		// Minus 1 because the original signature is also included.
		if (redirectSignatures.size() - 1 > maxRedirects) {

	public class RedirectSignatureTest {
	class RedirectSignatureTest {

Improve incorrect cyclic redirect detection logic #5548

Improve incorrect cyclic redirect detection logic #5548

Conversation

moromin commented Mar 29, 2024

CLAassistant commented Mar 29, 2024 • edited

github-actions bot commented Mar 29, 2024 • edited

🔍 Build Scan® (commit: 68c60ca)

codecov bot commented Mar 29, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moromin Apr 3, 2024 • edited

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

CLAassistant commented Mar 29, 2024 •

edited

github-actions bot commented Mar 29, 2024 •

edited

🔍 Build Scan® (commit: `68c60ca`)

codecov bot commented Mar 29, 2024 •

edited

moromin Apr 3, 2024 •

edited