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

HDFS-17040. Namenode web UI should set content type to application/octet-stream when uploading a file. #5721

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

zeroflag
Copy link
Contributor

@zeroflag zeroflag commented Jun 6, 2023

Description of PR

HDFS-17040

When uploading a file WebHDFS will set the content type to application/x-www-form-urlencoded, as this is the default used by jQuery

This causes knox to urlencode the request body so that uploading a CVS file 1,2,3 will result 1%2C2%2C3.

Instead of application/x-www-form-urlencoded I think the encoding should be set to application/octet-stream.

How was this patch tested?

Pending

For code changes:

content type is explicitly set to octet stream

@hadoop-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 48s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 codespell 0m 0s codespell was not available.
+0 🆗 detsecrets 0m 0s detect-secrets was not available.
+0 🆗 jshint 0m 0s jshint was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
_ trunk Compile Tests _
+1 💚 mvninstall 36m 14s trunk passed
+1 💚 shadedclient 58m 54s branch has no errors when building and testing our client artifacts.
_ Patch Compile Tests _
+1 💚 mvninstall 1m 8s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 shadedclient 22m 33s patch has no errors when building and testing our client artifacts.
_ Other Tests _
+1 💚 asflicense 0m 35s The patch does not generate ASF License warnings.
87m 19s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5721/1/artifact/out/Dockerfile
GITHUB PR #5721
Optional Tests dupname asflicense shadedclient codespell detsecrets jshint
uname Linux 73f0272140a2 4.15.0-206-generic #217-Ubuntu SMP Fri Feb 3 19:10:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/bin/hadoop.sh
git revision trunk / 395e833
Max. process+thread count 530 (vs. ulimit of 5500)
modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5721/1/console
versions git=2.25.1 maven=3.6.3
Powered by Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

@zeroflag zeroflag marked this pull request as draft June 6, 2023 12:52
@@ -518,7 +518,8 @@
url: url,
data: file.file,
processData: false,
crossDomain: true
crossDomain: true,
contentType: 'application/octet-stream'
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is this a js running in client browsers? The change here is to change the contentType when uploading a file from a client browser to HDFS. Is it correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@szetszwo Yes, this is the JS code running in the client browser. By default the contentType is set to application/x-www-form-urlencoded now it's explicitly set to application/octet-stream.

However I a found a problem when testing with newer browsers with CORS enabled. It works when I try it via Knox or with a browser where CORS is disabled.

But in new browsers with Cross-Origin Resource Sharing policy prevents changing the contentType since it's not included in the Access-Control-Allow-Headers header. Only the accept header is included in Access-Control-Allow-Headers.

Access-Control-Allow-Headers: Accept
Access-Control-Allow-Methods: PUT
Access-Control-Allow-Origin: *

The CrossOriginFilter contains the content type so I don't know why I only see Accept

Copy link
Contributor

Choose a reason for hiding this comment

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

The list is the default values. It can be overridden by init parameter; see

private void initializeAllowedHeaders(FilterConfig filterConfig) {
String allowedHeadersConfig =
filterConfig.getInitParameter(ALLOWED_HEADERS);
if (allowedHeadersConfig == null) {
allowedHeadersConfig = ALLOWED_HEADERS_DEFAULT;
}
allowedHeaders.addAll(
Arrays.asList(allowedHeadersConfig.trim().split("\\s*,\\s*")));
LOG.info("Allowed Headers: " + getAllowedHeadersHeader());
}

Copy link
Contributor Author

@zeroflag zeroflag Jun 7, 2023

Choose a reason for hiding this comment

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

Yes I've seen that, but it wasn't overridden. Even if I explicitly added

<property>
    <name>hadoop.http.cross-origin.allowed-headers</name>
    <value>X-Requested-With,Content-Type,Accept,Origin</value>
  </property>

to core-site.xml, it was not picked up for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

@szetszwo I'm not sure the CORS problem still exists. I've tested this using Chrome browser and with maximum security enabled the change was still working.

@szetszwo szetszwo marked this pull request as ready for review September 23, 2024 16:01
@szetszwo szetszwo changed the title HDFS-17040 - WebHDFS UI should set content type to application/octet-stream when uploading a file HDFS-17040. Namenode web UI should set content type to application/octet-stream when uploading a file. Sep 23, 2024
@@ -518,7 +518,8 @@
url: url,
data: file.file,
processData: false,
crossDomain: true
crossDomain: true,
contentType: 'application/octet-stream'
Copy link
Contributor

Choose a reason for hiding this comment

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

@szetszwo I'm not sure the CORS problem still exists. I've tested this using Chrome browser and with maximum security enabled the change was still working.

@Galsza
Copy link
Contributor

Galsza commented Sep 23, 2024

Thanks for the patch, looking good to me +1

I've tested this change in a cluster by trying to upload a file containing the text "hello %" via namenode ui. The change is indeed working. I haven't found a CORS related problem or at least Chrome browsers security levels don't seem to affect it.

@Galsza
Copy link
Contributor

Galsza commented Sep 23, 2024

@szetszwo could you please take another look at this PR?

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

+1 the change looks good.

@Galsza , thanks a lot for testing this!

@szetszwo
Copy link
Contributor

Since this pull request already passed all the checks earlier, let's just merge it.

@szetszwo szetszwo merged commit 6831574 into apache:trunk Sep 23, 2024
1 check failed
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.

4 participants