Skip to content

Conversation

@xdjoshuaaz
Copy link
Contributor

@xdjoshuaaz xdjoshuaaz commented Jun 11, 2023

The change made in PR #7514 assumes that custom ports work with HTTP requests, however, this is not true.

PR #5255 did correctly mention the dev-time discrepancy but it was not merged.

Weirdly, it is possible to open up a TCP socket to the HTTP server on the custom port using the recent TCP sockets runtime API, and craft a HTTP request + parse the HTTP response by hand.

Because of this, the decision to restrict custom ports when using fetch may be now be defeated by this new TCP socket API?

The change made in PR cloudflare#7514 assumes that custom ports work with HTTP requests, however, this is not true.

PR cloudflare#5255 did correctly mention the dev-time discrepancy but it was not merged.
@github-actions github-actions bot requested a review from deadlypants1973 June 11, 2023 11:10
@deadlypants1973 deadlypants1973 changed the title Update "Custom Ports" known issue to reflect dev-time only behaviour [Workers] Update "Custom Ports" known issue to reflect dev-time only behaviour Jun 15, 2023
@KimJ15 KimJ15 added the product:workers Related to Workers product label Jun 21, 2023
Copy link
Contributor

@irvinebroque irvinebroque left a comment

Choose a reason for hiding this comment

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

Thanks for flagging this. Small copy edits but 🚀

@mrbbot @admah @lrapoport-cf for visibility — wonder if we should update local dev / Quick Editor to more closely mirror production behavior.

@mrbbot
Copy link
Contributor

mrbbot commented Jul 26, 2023

@irvinebroque tempted to leave it as-is in local dev. We log a warning when you try to fetch on a non-standard port, but being able to do this locally is very useful for accessing dev servers that don't usually run on standard ports.

Copy link
Contributor

@deadlypants1973 deadlypants1973 left a comment

Choose a reason for hiding this comment

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

PCX

@irvinebroque irvinebroque requested review from a team, GregBrimble and WalshyDev as code owners July 26, 2023 16:36
@irvinebroque irvinebroque merged commit 53b5569 into cloudflare:production Jul 26, 2023
@admah
Copy link
Contributor

admah commented Jul 26, 2023

+1 to the comment from @mrbbot. As long as it's documented here, I'm good with leaving the local dev experience as-is.

yusukebe pushed a commit that referenced this pull request Aug 29, 2023
…behaviour (#9324)

* Update "Custom Ports" known issue to reflect dev-time only behaviour

The change made in PR #7514 assumes that custom ports work with HTTP requests, however, this is not true.

PR #5255 did correctly mention the dev-time discrepancy but it was not merged.

* Apply suggestions from code review

---------

Co-authored-by: Brendan Irvine-Broque <brendanib@gmail.com>
Co-authored-by: Brendan Irvine-Broque <birvine-broque@cloudflare.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

product:workers Related to Workers product size/xs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants