GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module by hamzaavvan · Pull Request #24848 · python/cpython

bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module #24848

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.

Sign up for GitHub

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

Closed
hamzaavvan wants to merge 1 commit into python:main from hamzaavvan:fix-issue-43223

Uh oh!

There was an error while loading. Please reload this page.
Closed

bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module #24848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
42eb552
bpo-43223: Fix Open Redirection In http.server module
hamzaavvan Mar 13, 2021
File filter
Filter by extension
Clear filters
Conversations
Failed to load comments.
Loading

Uh oh!

There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading

Uh oh!

There was an error while loading. Please reload this page.
Diff view
Diff view

There are no files selected for viewing

7 changes: 7 additions & 0 deletions 7 Lib/http/server.py
Show comments
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
import socket # For gethostbyaddr()
import socketserver
import sys
import re
import time
import urllib.parse
import contextlib
Expand Down Expand Up @@ -332,6 +333,12 @@ def parse_request(self):
return False
self.command, self.path = command, path

# bpo-43223: The purpose of replacing '//' with '/' is to protect against
# open redirect attacks reside within http.server module which can be triggered
# if the path contains '//' at the beginning because web clients treat //path as
# an absolute url without scheme (similar to http://path) rather than a relative path
self.path = re.sub(r'^(/)+', '/', self.path)

# Examine the headers and look for a Connection directive.
try:
self.headers = http.client.parse_headers(self.rfile,
Expand Down
11 changes: 11 additions & 0 deletions 11 Lib/test/test_http/test_http.py
Show comments
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import unittest
import re

class TestHTTP(unittest.TestCase):

def test_http_parse_request(self):
self.assertEqual(re.sub(r'^/+', '/', '//test.com'), '/test.com', '//test.com should be converted to a proper relative path')
self.assertEqual(re.sub(r'^/+', '/', '///test.com'), '/test.com', '///test.com should be converted to a proper relative path')
Comment thread

@ambv ambv Jul 12, 2021

Copy link
Copy Markdown
Contributor

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.

This doesn't test anything useful. It only checks that some regular expression matches two strings. Since your code change is in http.server, your test should execute the http.server path that you changed.

A good test should fail without your code change to http.server, and pass with your change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
merwok and lofidef reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@hamzaavvan hamzaavvan Aug 21, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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.

Sorry I've been away due to my workload. But now as I've got some spare time I can do it better. As I've already said earlier that I'm new to writing these test cases and after checking out your comment I'm still confused about your requirements

Can you please elaborate these two statements:

Since your code change is in http.server, your test should execute the http.server path that you changed.

and this one:

A good test should fail without your code change to http.server, and pass with your change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ambv ambv Aug 29, 2021

Copy link
Copy Markdown
Contributor

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.

@hamzaavvan, to put it in the simplest terms:

  • on a clean checkout of the main branch, write a test that fails because it triggers the problem described in BPO-43223; this will prove we have a problem;
  • then include your fix to prove that the fix solved the problem.

The test will have to actually run the HTTP server to be useful.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

if __name__ == '__main__':
unittest.main()
2 changes: 2 additions & 0 deletions 2 Misc/NEWS.d/next/Security/2021-03-13-21-25-29.bpo-43223.ieBVWq.rst
Show comments
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
:mod:`http.server`: Fix an open redirection vulnerability in the HTTP server when an URL contains ``//``.
Vulnerability discovered and fixed by Hamza Avvan.
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.