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

Http uri spaces 2881 v13 #8169

Closed
wants to merge 2 commits into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/2881

Describe changes:

  • configures libhtp to allow spaces in URIs

Modifies #8134 with rebase

libhtp-pr: 371
suricata-verify-pr: 1004

OISF/libhtp#371
OISF/suricata-verify#1004

@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #8169 (939caa1) into master (a4239d4) will increase coverage by 0.09%.
The diff coverage is 83.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8169      +/-   ##
==========================================
+ Coverage   81.73%   81.82%   +0.09%     
==========================================
  Files         962      962              
  Lines      276975   277019      +44     
==========================================
+ Hits       226379   226684     +305     
+ Misses      50596    50335     -261     
Flag Coverage Δ
fuzzcorpus 63.72% <83.33%> (+0.19%) ⬆️
suricata-verify 59.44% <77.77%> (+0.01%) ⬆️
unittests 63.37% <22.22%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

WARNING:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field baseline test %
SURI_TLPW1_stats_chk
.tcp.rst 128237 103898 81.02%

Pipeline 10607

Comment on lines +39 to +42
- Spaces are accepted in HTTP1 URIs instead of in the protocol version

That is `GET /a b HTTP/1.1` gets now URI as `a b` and protocol as `HTTP/1.1` when
it used to be URI as `a` and protocol as `b HTTP/1.1`
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is a bit confusing... I think that two things could be done.

i) bring the next sentence together to the item list that starts this topic.
ii) adjust the text a bit.

Resulting in:

- Spaces are accepted in HTTP1 URIs instead of in the protocol version. That is: 
  `GET /a b HTTP/1.1` gets now URI as `a b` and protocol as `HTTP/1.1`. Before,
  this would result in URI as `a` and protocol as `b HTTP/1.1`.

Copy link
Member

Choose a reason for hiding this comment

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

uri would be /a b not a b (/ is part of the uri)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

Added a suggestion in the documentation, to improve readability. :)

@catenacyber
Copy link
Contributor Author

Replaced by #8252

@catenacyber catenacyber closed this Dec 7, 2022
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