-
Notifications
You must be signed in to change notification settings - Fork 235
fix: ensure IP restriction error handling for run:inside and logs commands #3389
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.
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
Open
michaelmalave
wants to merge
7
commits into
main
Choose a base branch
from
mm/chore/uniform-errors-outside-trusted-ips
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ands. Prevents silent failures when commands are executed outside trusted IP ranges
| // For 403 errors, distinguish between stream expiration and IP restrictions | ||
| if (isTail) { | ||
| // When tailing, 403 typically means stream access expired | ||
| msg = 'Log stream access expired. Please try again.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
| msg = 'Log stream access expired. Please try again.' | |
| msg = 'Your access to the log stream expired. Try again.' |
…tside-trusted-ips
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here's the updated PR template with the details of this change:
Summary
Fixed silent failure issue in heroku run:inside and improved error messages in heroku logs when executed outside trusted IP ranges. Added uniform error handling for 403 HTTP responses to provide clear error messages instead of silent failures or generic errors.
Type of Change
Note: Add a
!after your change type to denote a breaking change.Testing
Follow guide for staging heroku environment here: staging setup.
get current webserver
Confirm ip restrictions (should be empty):
Run commands against test space + app with IP restrictions enabled:
Both should return the same error:
Error: You can't access this app from your IP addressAdditional Context
Root Cause:
run:insidecommand failed silently when IP restrictions were active because the HTTPS request in the SSH connection path received a 403 response but had no handler for it, causing the Promise to hang indefinitely waiting for anupgradeevent that never came.logscommand displayed generic error messages for 403 responses because EventSource doesn't expose HTTP response bodies, making it impossible to distinguish between IP restrictions and stream expiration errors.Fix Applied:
dyno.ts: Added HTTP response handler in_connect()method to check for 403 status codes and reject the Promise with a user-friendly error message: "You can't access this space from your IP address. Contact your team admin."log-displayer.ts: ImplementedfetchHttpResponseBody()utility function that makes a separate HTTP request to fetch the actual response body (since EventSource doesn't expose it). The error handler now:Implementation Details:
httpsmodule directly (matching existing patterns indyno.ts) to handle staging SSL certificatesBoth commands now provide clear, actionable error messages that match the server's response, maintaining a consistent user experience across commands.
Resolved usage:

Related Issue
Closes W-19883467