Skip to content

Improve HTTP redirect following#1637

Open
bioball wants to merge 6 commits into
apple:mainfrom
bioball:manage-http-redirects
Open

Improve HTTP redirect following#1637
bioball wants to merge 6 commits into
apple:mainfrom
bioball:manage-http-redirects

Conversation

@bioball
Copy link
Copy Markdown
Member

@bioball bioball commented Jun 2, 2026

This implements HTTP redirect following ourselves.

The goal is:

  1. All I/O is checked against --allowed-resources and --allowed-modules, including HTTP redirects
  2. HTTP rewrite rules can affect redirect following
  3. HTTP headers can affect redirect following

return response;
}
}
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This class (RequestRewritingClient) maybe should be renamed. Alternatively, we can implement the redirect following outside of HttpClient (like in HttpUtils).

I don't feel strongly either way, but, food for thought for the future.

This implements HTTP redirect following ourselves.

The goal is:

1. All I/O is checked against `--allowed-resources` and `--allowed-modules`,
  including HTTP redirects
2. HTTP rewrite rules can affect redirect following
3. HTTP headers can affect redirect following
@bioball bioball force-pushed the manage-http-redirects branch from 70d95af to 5967a95 Compare June 2, 2026 23:38
Comment thread pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java Outdated
Comment thread pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java Outdated
Comment thread pkl-core/src/main/java/org/pkl/core/util/HttpUtils.java
Comment thread pkl-core/src/main/resources/org/pkl/core/errorMessages.properties Outdated
Comment thread pkl-core/src/main/resources/org/pkl/core/errorMessages.properties Outdated
@bioball bioball force-pushed the manage-http-redirects branch from 50f08ca to 2b1100b Compare June 3, 2026 18:33
Comment thread pkl-core/src/main/java/org/pkl/core/runtime/ResourceManager.java Outdated
Copy link
Copy Markdown
Contributor

@HT154 HT154 left a comment

Choose a reason for hiding this comment

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

LGTM. We definitely need to document this as a breaking change as previously-valid "inner" redirects may have worked but will now fail. We should update the documentation to reflect the precise behavior here.

@HT154
Copy link
Copy Markdown
Contributor

HT154 commented Jun 4, 2026

Does this PR address #1163?

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.

2 participants