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.

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

Remove path from 404 response #3165

Merged
merged 2 commits into from
Apr 22, 2021
Merged

Conversation

kmcgrady
Copy link
Collaborator

Browsers attempt to render the path, which doesn't make any sense. It's best to just remove it.

@kmcgrady kmcgrady requested a review from a team April 21, 2021 21:35
tconkling
tconkling approved these changes Apr 21, 2021
@@ -327,7 +327,7 @@ def get(self, path: str) -> None:
with open(abspath, "r", encoding="utf-8") as file:
contents = file.read()
except (OSError, UnicodeDecodeError) as e:
self.write(f"{path} read error: {e}")
self.write(f"read error: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

To be extra safe, I think we should also remove the exception from the interpolated string here. (So just: self.write("read error")). We don't need to leak details to the browser about what's going on under the hood. (We could log the exception to the console, for the site admin to see.)

@kmcgrady kmcgrady merged commit afcf880 into streamlit:develop Apr 22, 2021
@kmcgrady kmcgrady deleted the remove-path branch April 22, 2021 00:28
tconkling added a commit that referenced this pull request Apr 26, 2021
* develop:
  Support binary files (#3144)
  Secrets Management docs (#3065)
  Consolidated startup messages into bootstrap.py and made them pretty. (#3162)
  Merge Small Text feature branch (#3168)
  Stick our script-completion logic into a documented method (#3166)
  Remove path from 404 response (#3165)
  Merge in Git Deploy Menu Item feature (#3154)
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.

None yet

2 participants