Copilot commented on code in PR #536:
URL: https://github.com/apache/tez/pull/536#discussion_r4046798058
##########
tez-plugins/tez-aux-services/src/main/java/org/apache/tez/auxservices/ShuffleHandler.java:
##########
@@ -1167,6 +1195,41 @@ private boolean isNullOrEmpty(List<String> entries) {
return entries == null || entries.isEmpty();
}
+ /**
+ * Validate the structural query parameters (dag, vertex, map/attempt) that
+ * are concatenated into filesystem paths, rejecting anything containing a
+ * path-traversal component so the request cannot escape the per-app
+ * shuffle directory. Returns true when the request may proceed and false
+ * when it has already been closed with a 400 response.
+ */
+ private boolean validateShufflePathParams(ChannelHandlerContext ctx,
+ List<String> dagIdQ, List<String> vertexIdQ, List<String> mapIds,
+ boolean isDeleteRequest) {
+ if (dagIdQ != null && !dagIdQ.isEmpty()) {
+ String dagId = dagIdQ.get(0);
+ if (dagId == null || !DAG_ID_PATTERN.matcher(dagId).matches()) {
+ sendError(ctx, "Bad dag parameter", BAD_REQUEST);
+ return false;
+ }
+ }
+ if (vertexIdQ != null && !vertexIdQ.isEmpty()) {
+ String vertexId = vertexIdQ.get(0);
+ if (vertexId == null ||
!VERTEX_ID_PATTERN.matcher(vertexId).matches()) {
+ sendError(ctx, "Bad vertex parameter", BAD_REQUEST);
+ return false;
+ }
+ }
Review Comment:
validateShufflePathParams only checks the first dag/vertex value and will
accept multiple occurrences (e.g., dag=1&dag=2), which contradicts the goal of
rejecting unexpected parameter shapes up front. It should reject any request
where dag/vertex are present more than once.
##########
tez-plugins/tez-aux-services/src/test/java/org/apache/tez/auxservices/TestShuffleHandler.java:
##########
@@ -1810,6 +1810,123 @@ public FullHttpRequest createHttpRequest() {
return new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET,
uri);
}
+ /**
+ * dag, vertex and map query parameters are concatenated into filesystem
+ * paths inside the shuffle handler. A value carrying a path-traversal
+ * component must be refused so it cannot escape the per-app shuffle
+ * directory and drive an out-of-scope read or delete.
+ */
+ @Test
+ @Timeout(value = 10000, unit = TimeUnit.MILLISECONDS)
+ public void testTraversalInDagVertexMapIsRejected() throws Exception {
+ Configuration conf = getInitialConf();
+ conf.setInt(ShuffleHandler.MAX_SHUFFLE_CONNECTIONS, 3);
+ conf.set(CommonConfigurationKeysPublic.HADOOP_SECURITY_AUTHENTICATION,
+ "simple");
+ UserGroupInformation.setConfiguration(conf);
+ conf.set(YarnConfiguration.NM_LOCAL_DIRS, TEST_DIR.getAbsolutePath());
+ ApplicationId appId = ApplicationId.newInstance(12345, 1);
+ String appAttemptId = "attempt_12345_1_m_1_0";
+ String user = "randomUser";
+ List<File> fileMap = new ArrayList<File>();
+ createShuffleHandlerFiles(TEST_DIR, user, appId.toString(), appAttemptId,
+ conf, fileMap);
+ ShuffleHandler shuffleHandler = new ShuffleHandler() {
+ private AuxiliaryLocalPathHandler pathHandler = new
TestAuxiliaryLocalPathHandler();
+ @Override
+ protected Shuffle getShuffle(Configuration conf) {
+ return new Shuffle(conf) {
+ @Override
+ protected void verifyRequest(String appid, ChannelHandlerContext ctx,
+ HttpRequest request, HttpResponse response, URL requestUri)
+ throws IOException {
+ // Traversal must be rejected regardless of authentication state.
+ }
+ };
+ }
+ @Override
+ public AuxiliaryLocalPathHandler getAuxiliaryLocalPathHandler() {
+ return pathHandler;
+ }
+ };
+ shuffleHandler.init(conf);
+ try {
+ shuffleHandler.start();
+ DataOutputBuffer outputBuffer = new DataOutputBuffer();
+ outputBuffer.reset();
+ Token<JobTokenIdentifier> jt =
+ new Token<JobTokenIdentifier>("identifier".getBytes(),
+ "password".getBytes(), new Text(user), new
Text("shuffleService"));
+ jt.write(outputBuffer);
+ shuffleHandler
+ .initializeApplication(new ApplicationInitializationContext(user,
+ appId, ByteBuffer.wrap(outputBuffer.getData(), 0,
+ outputBuffer.getLength())));
+ String base = "http://127.0.0.1:";
+ +
shuffleHandler.getConfig().get(ShuffleHandler.SHUFFLE_PORT_CONFIG_KEY);
+
+ // File that must survive every attempt below.
+ File outside = new File(TEST_DIR, "outside.txt");
+ try (FileOutputStream out = new FileOutputStream(outside)) {
+ out.write("keep me\n".getBytes());
+ }
+ assertTrue(outside.exists());
+
+ // dagAction=delete with a traversing dag value must not delete anything
+ // and must not return OK.
+ String badDag = URI.create("http:///a";).resolve(
+ "?dagAction=delete&job=job_12345_0001&dag=1/../../..").getRawQuery();
+ HttpURLConnection conn = (HttpURLConnection) URI.create(
+ base + "/mapOutput?" + badDag).toURL().openConnection();
+ conn.setRequestProperty(ShuffleHeader.HTTP_HEADER_NAME,
+ ShuffleHeader.DEFAULT_HTTP_HEADER_NAME);
+ conn.setRequestProperty(ShuffleHeader.HTTP_HEADER_VERSION,
+ ShuffleHeader.DEFAULT_HTTP_HEADER_VERSION);
+ conn.connect();
+ // The server must reject the request; exact status is a bad-request
+ // family response, never a successful delete.
+ int code = conn.getResponseCode();
+ assertTrue(code >= 400 && code < 600,
+ "Expected an error response for traversing dag, got " + code);
+ assertTrue(outside.exists(),
+ "outside.txt must not be deleted by a traversing dag delete");
+
+ // vertexAction=delete with a traversing vertex value must be rejected.
+ conn = (HttpURLConnection) URI.create(
+ base +
"/mapOutput?vertexAction=delete&job=job_12345_0001&dag=1&vertex=00/../"
+ ).toURL().openConnection();
+ conn.setRequestProperty(ShuffleHeader.HTTP_HEADER_NAME,
+ ShuffleHeader.DEFAULT_HTTP_HEADER_NAME);
+ conn.setRequestProperty(ShuffleHeader.HTTP_HEADER_VERSION,
+ ShuffleHeader.DEFAULT_HTTP_HEADER_VERSION);
+ conn.connect();
+ code = conn.getResponseCode();
+ assertTrue(code >= 400 && code < 600,
+ "Expected an error response for traversing vertex, got " + code);
+ assertTrue(outside.exists(),
+ "outside.txt must not be deleted by a traversing vertex delete");
+
+ // A shuffle read with a traversing map= value must not resolve to a
+ // path outside the app's shuffle output directory. The pathCache loader
+ // will refuse to load such an attempt id.
+ conn = (HttpURLConnection) URI.create(
+ base + "/mapOutput?job=job_12345_1&dag=1&reduce=1&map="
Review Comment:
This request is intended to exercise traversal rejection in the map
parameter, but it uses a different job id format (job_12345_1) than the
surrounding shuffle handler tests (job_12345_0001). Using the consistent,
known-good job id reduces the chance the test passes for an unrelated
validation failure if request parsing/validation order changes.
##########
tez-plugins/tez-aux-services/src/main/java/org/apache/tez/auxservices/ShuffleHandler.java:
##########
@@ -1167,6 +1195,41 @@ private boolean isNullOrEmpty(List<String> entries) {
return entries == null || entries.isEmpty();
}
+ /**
+ * Validate the structural query parameters (dag, vertex, map/attempt) that
+ * are concatenated into filesystem paths, rejecting anything containing a
+ * path-traversal component so the request cannot escape the per-app
+ * shuffle directory. Returns true when the request may proceed and false
+ * when it has already been closed with a 400 response.
+ */
+ private boolean validateShufflePathParams(ChannelHandlerContext ctx,
+ List<String> dagIdQ, List<String> vertexIdQ, List<String> mapIds,
+ boolean isDeleteRequest) {
Review Comment:
validateShufflePathParams takes an isDeleteRequest parameter but never uses
it, which adds noise and makes it harder to tell what is actually being
validated. Either remove the parameter (and update the call site) or use it for
request-shape validation that differs between delete vs. read requests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
