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

Headers modified in Socket creation. #179

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Headers modified in Socket creation. #179

merged 1 commit into from
Dec 7, 2021

Conversation

bparmar-splunk
Copy link
Contributor

@bparmar-splunk bparmar-splunk commented Dec 1, 2021

Update:

  • HTTP Request using format strings to insert user supplied data into the raw requests headers. So to prevent attackers to replace escape characters in request string. It is recommended to use the PrintWriter which allows us to bifurcate headers using println method instead of manually entering \r\n characters.
  • Host in socket is removed as it should not be exposed to external users from request headers.

Update:
- String formatter with \r\n removed and modified with alternate approach.
- Host param is removed.
@@ -83,8 +83,7 @@ public Socket attach(Args args) throws IOException {
*/
public Socket attach(String indexName, Args args) throws IOException {
Socket socket = service.open();
OutputStream ostream = socket.getOutputStream();
Writer out = new OutputStreamWriter(ostream, "UTF-8");
PrintWriter writer = new PrintWriter(new OutputStreamWriter(socket.getOutputStream(), "UTF-8"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the type of writer being changed here? It's not clear from the PR summary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR summary updated.


out.write(header.toString());
out.flush();
headers.add("");
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious what this line is doing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This empty string for headers is intentionally kept for terminating header string

Copy link
Contributor

@fantavlik fantavlik left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the explanations @bparmar-splunk 🚀

@bparmar-splunk bparmar-splunk merged commit 9af4c72 into develop Dec 7, 2021
@bparmar-splunk bparmar-splunk deleted the DVPL-7634 branch December 15, 2021 07:08
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.

2 participants