Further fix validate path on Windows #10330
Conversation
Signed-off-by: Axel Chong <haxatron1@gmail.com>
|
Documentation preview for 06dcac1 will be available here when this CircleCI job completes successfully. More info
|
github-actions
bot
added
area/tracking
|
@serena-ruan @harupy, as you reviewed #8999, can you review this? |
| r"C:path", | ||
| r"C:path/", | ||
| r"C:path/to/file", | ||
| r"C:../path/to/file", |
There was a problem hiding this comment.
I get r"C:../path/to/file" should be considered as bad path, but why the first three also bad? Relative path without .. should be good right?
There was a problem hiding this comment.
When I tested on the mlartifacts API end point, using drive-relative path without .. will break out of mlartifacts directory for some reason (ie. instead of searching in dir/mlartifacts, MLFlow search in dir instead). Also, since it seems very edge-cased that someone would use these sort of paths, I decided to disallow them outright.
There was a problem hiding this comment.
Disallowing C:/path/to/file is a behavior change if it's supposed to be a safe path. Could we just disallow '..' by splitting the path on ':' as well?
There was a problem hiding this comment.
What I meant above is that in C:path/to/file is also unsafe because in the /mlflow-artifacts/artifacts/ endpoint
/mlflow-artifacts/artifacts/C:path/to/file -> for some reason will access path/to/file instead of mlartifacts/path/to/file, which is unintended (we only want access to mlartifacts directory)
Therefore since it is unsafe, I decided to disallow these paths as it is extremely extremely unlikely to be used because:
-
People would use
/mlflow-artifacts/artifacts/path/to/fileinstead of/mlflow-artifacts/artifacts/C:path/to/fileto access the file they want. -
Knowledge of these relative paths with drive C: (ie
C:path/to/file) is quite obscure.
There was a problem hiding this comment.
/mlflow-artifacts/artifacts/C:path/to/file -> for some reason will access path/to/file instead of mlartifacts/path/to/file, which is unintended (we only want access to mlartifacts.)
This does look weird, they could just remove 'C:' for accessing relative paths. Thanks for the explanation :D
Signed-off-by: swathi <konakanchi.swathi@gmail.com>
馃洜 DevTools 馃洜
Install mlflow from this PR
Checkout with GitHub CLI
Related Issues/PRs
#xxxWhat changes are proposed in this pull request?
Follow up of #8999.
On Windows, drive-relative paths without slash after drive letter and colon (ie. C:path/to/file) will be converted to a relative path.
So,
C:path/to/file->path/to/fileThis has security implication when there is a .. in the path.
C:../path/to/file->../path/to/fileThis PR fixes this scenario by checking whether the 2nd character of the path provided is a
:Reference: https://stackoverflow.com/questions/23955968/windows-path-with-no-slash-after-drive-letter-and-colon-what-does-it-point-to
How is this PR tested?
Does this PR require documentation update?
Release Notes
Is this a user-facing change?
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/artifacts: Artifact stores and artifact loggingarea/build: Build and test infrastructure for MLflowarea/docs: MLflow documentation pagesarea/examples: Example codearea/gateway: AI Gateway service, Gateway client APIs, third-party Gateway integrationsarea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/models: MLmodel format, model serialization/deserialization, flavorsarea/recipes: Recipes, Recipe APIs, Recipe configs, Recipe Templatesarea/projects: MLproject format, project running backendsarea/scoring: MLflow Model server, model deployment tools, Spark UDFsarea/server-infra: MLflow Tracking server backendarea/tracking: Tracking Service, tracking client APIs, autologgingInterface
area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Modelsarea/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registryarea/windows: Windows supportLanguage
language/r: R APIs and clientslanguage/java: Java APIs and clientslanguage/new: Proposals for new client languagesIntegrations
integrations/azure: Azure and Azure ML integrationsintegrations/sagemaker: SageMaker integrationsintegrations/databricks: Databricks integrationsHow should the PR be classified in the release notes? Choose one:
rn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notes