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

Provide custom 404 view that doesn't echo path #800

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

sp3nx0r
Copy link
Contributor

@sp3nx0r sp3nx0r commented Jul 24, 2023

💸 TL;DR

We get some bogus bug bounty submissions that talk about HTML injection on our 404 Not Found pages, which are default Pyramid views which echo out the path of the Request that isn't found. So you can put some gibberish in there, but it looks real jank and would not be a valid social engineering attack, but it should be simple for us to just default this to be quieter so we don't receive these reports.

Ref: https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/hooks.html

📜 Details

Example: https://ads-api.reddit.com/this_is_a_test_where_i_could_spoof_whatever_i_guess

🧪 Testing Steps / Validation

TBD

✅ Checks

  • CI tests (if present) are passing
  • Adheres to code style for repo
  • Contributor License Agreement (CLA) completed if not a Reddit employee

@sp3nx0r sp3nx0r requested a review from a team as a code owner July 24, 2023 14:50
@maeivysea maeivysea requested review from scotthew1 and removed request for maeivysea July 24, 2023 16:47
@maeivysea
Copy link
Contributor

👋 deferring to our baseplate SMEs

@ketralnis ketralnis self-requested a review July 24, 2023 16:58
@ketralnis
Copy link
Contributor

ketralnis commented Jul 24, 2023

I'm not able to push up into this branch but here's the whole diff after the fix and the added test. I've confirmed that the test fails before and passes after

diff --git a/baseplate/frameworks/pyramid/__init__.py b/baseplate/frameworks/pyramid/__init__.py
index eba262a..842931d 100644
--- a/baseplate/frameworks/pyramid/__init__.py
+++ b/baseplate/frameworks/pyramid/__init__.py
@@ -412,6 +412,7 @@ class BaseplateConfigurator:
         config.set_request_factory(RequestFactory(self.baseplate))
         config.add_subscriber(self._on_new_request, pyramid.events.ContextFound)
         config.add_subscriber(self._on_application_created, pyramid.events.ApplicationCreated)
+        config.add_notfound_view(notfound_override)

         # Position of the tween is important. We need it to cover all code
         # that can written in the app. This means that it should be above
@@ -444,3 +445,7 @@ def get_is_healthy_probe(request: Request) -> int:
             code,
         )
         return IsHealthyProbe.READINESS
+
+
+def notfound_override(_request: Request) -> Response:
+    return Response("Not Found", status="404 Not Found")
diff --git a/tests/integration/pyramid_tests.py b/tests/integration/pyramid_tests.py
index 2f85b7b..b4581ce 100644
--- a/tests/integration/pyramid_tests.py
+++ b/tests/integration/pyramid_tests.py
@@ -194,6 +194,14 @@ class ConfiguratorTests(unittest.TestCase):
         self.assertFalse(self.observer.on_server_span_created.called)
         self.assertFalse(self.context_init_event_subscriber.called)

+    def test_not_found_echo_path(self):
+        # confirm that issue #800 isn't reintroduced. This is an issue where we
+        # echo the path to the 404 in the response which probably isn't a
+        # problem, but does show up in automated vuln scans which can cause some
+        # extra work hunting down false positives
+        resp = self.test_app.get("/doesnt_exist", status=404)
+        self.assertNotIn(b"doesnt_exist", resp.body)
+
     def test_exception_caught(self):
         with self.assertRaises(TestException):
             self.test_app.get("/example?error")

We get some bogus bug bounty submissions that talk about HTML injection
on our 404 Not Found pages, which are default Pyramid views which echo
out the `path` of the Request that isn't found. So you can put some
gibberish in there, but it looks real jank and would not be a valid
social engineering attack, but it should be simple for us to just
default this to be quieter so we don't receive these reports.

Ref: https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/hooks.html
Example: https://ads-api.reddit.com/this_is_a_test_where_i_could_spoof_whatever_i_guess

- [x] CI tests (if present) are passing
- [x] Adheres to code style for repo
- [x] Contributor License Agreement (CLA) completed if not a Reddit employee
Copy link
Contributor

@ketralnis ketralnis left a comment

Choose a reason for hiding this comment

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

got the code swapped out and added a test as well. but this is now me approving my own code so it could use another look :)

@sp3nx0r
Copy link
Contributor Author

sp3nx0r commented Aug 7, 2023

@ketralnis 's code looks good to me, going to be merging before EOD

@sp3nx0r sp3nx0r merged commit 1142489 into reddit:develop Aug 7, 2023
3 checks passed
@sp3nx0r sp3nx0r deleted the default-404-no-route branch August 7, 2023 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants