Skip to content

Honor explicit ports in HTTPS checks#267

Merged
jsf9k merged 4 commits into
cisagov:developfrom
arpitjain099:chore/handle-explicit-https-ports
Jun 22, 2026
Merged

Honor explicit ports in HTTPS checks#267
jsf9k merged 4 commits into
cisagov:developfrom
arpitjain099:chore/handle-explicit-https-ports

Conversation

@arpitjain099

@arpitjain099 arpitjain099 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Fixes #254

This came from https_check always building the SSLyze target with port 443 after slicing the URL string. If someone scans domain:9443, the port was not handled correctly and cert trust or chain fields ended up null.

This updates target parsing to use urlparse and pass the parsed host plus parsed port to SSLyze. When no explicit port is present, it still defaults to 443.

I also added unit tests that assert calls to with_ip_address_lookup for:

  • example.com:9443 -> host example.com, port 9443
  • example.com -> host example.com, port 443

I could not run pytest end-to-end in this local environment because the nassl binary fails to load (_SSLv2_method symbol issue on macOS toolchain). I still verified changes by compiling modified files and running a targeted Python harness with stubbed SSLyze modules to validate host and port parsing.

@jsf9k jsf9k self-assigned this Jun 17, 2026
@jsf9k jsf9k added python Pull requests that update Python code bug This issue or pull request addresses broken functionality labels Jun 17, 2026
@github-project-automation github-project-automation Bot moved this to In progress in BOD 18-01 Jun 17, 2026
@jsf9k jsf9k moved this to In Progress in CyHy System Jun 17, 2026
@jsf9k jsf9k moved this from In progress to Review in progress in BOD 18-01 Jun 17, 2026
@jsf9k

jsf9k commented Jun 17, 2026

Copy link
Copy Markdown
Member

@arpitjain099 - Are the stack traces that appear when running the tests you added necessary? Is there any way to get rid of them? They misled me into thinking that tests had failed when I ran pytest locally.

@jsf9k

jsf9k commented Jun 17, 2026

Copy link
Copy Markdown
Member

@arpitjain099 - Are the stack traces that appear when running the tests you added necessary? Is there any way to get rid of them? They misled me into thinking that tests had failed when I ran pytest locally.

Hmmm...looks like the stack traces are indeed necessary because of the way that pshtt handles exceptions.

@jsf9k jsf9k added version bump This issue or pull request increments the version number test This issue or pull request adds or otherwise modifies test code labels Jun 17, 2026
@arpitjain099

Copy link
Copy Markdown
Contributor Author

Good catch, and they're not actually necessary - that was the test leaking pshtt's own exception logging. Both tests stop the scan early by raising from perform(), and https_check catches that and logs it with logging.exception, so the traceback printed even though the tests passed. I wrapped those calls in with self.assertLogs(level="ERROR"): so the expected error log is captured instead of printed. Pushed - output should be quiet now.

@jsf9k jsf9k enabled auto-merge June 17, 2026 17:26
arpitjain099 and others added 4 commits June 22, 2026 14:41
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
The two new tests stop the scan early by raising from perform(), which
https_check catches and logs via logging.exception. That dumped a traceback
into the pytest output even though the tests pass. Wrap the calls in
self.assertLogs(level=ERROR) so the expected error log is captured instead of
printed.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@jsf9k jsf9k force-pushed the chore/handle-explicit-https-ports branch from 4f43a6b to b9bf6d0 Compare June 22, 2026 18:41

@dav3r dav3r left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This all looks good, but I'd like to see one more test added: test_http_check_defaults_to_port_80 since that logic is being added in this PR as well.

@jsf9k jsf9k merged commit 6092402 into cisagov:develop Jun 22, 2026
37 checks passed
@github-project-automation github-project-automation Bot moved this from Review in progress to Reviewer approved in BOD 18-01 Jun 22, 2026
@github-project-automation github-project-automation Bot moved this from In Progress to Done in CyHy System Jun 22, 2026
@github-project-automation github-project-automation Bot moved this from Reviewer approved to Done in BOD 18-01 Jun 22, 2026
@dav3r

dav3r commented Jun 22, 2026

Copy link
Copy Markdown
Member

This all looks good, but I'd like to see one more test added: test_http_check_defaults_to_port_80 since that logic is being added in this PR as well.

Doh- I didn't notice that this PR had auto-merge enabled, otherwise I would've used the "Request Changes" option.

@arpitjain099

Copy link
Copy Markdown
Contributor Author

This all looks good, but I'd like to see one more test added: test_http_check_defaults_to_port_80 since that logic is being added in this PR as well.

Doh- I didn't notice that this PR had auto-merge enabled, otherwise I would've used the "Request Changes" option.

@dav3r no worries let me check

@arpitjain099

Copy link
Copy Markdown
Contributor Author

@dav3r opened #268 to add test_http_check_defaults_to_port_80. Thanks!

jsf9k pushed a commit that referenced this pull request Jun 24, 2026
Follow-up to #267, which also added the http -> port 80 default. This
covers that branch, mirroring the existing port 443 test.

Signed-off-by: arpitjain099 <arpitjain099@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This issue or pull request addresses broken functionality python Pull requests that update Python code test This issue or pull request adds or otherwise modifies test code version bump This issue or pull request increments the version number

Projects

Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

Ports other than 443 result in null values where there shouldn't be null values

3 participants