Allow StaticFiles follow symlinks #1683

aminalaee · 2022-06-10T13:23:29Z

After the previous attempt #1377 , this PR will allow StaticFiles to follow symlinks outside the directory.

Add test to prevent path traversal Update requirements.txt Update tests

aminalaee · 2022-06-10T14:37:38Z

starlette/staticfiles.py

+            full_path = os.path.abspath(os.path.join(directory, path))
            directory = os.path.realpath(directory)
            if os.path.commonprefix([full_path, directory]) != directory:
                # Don't allow misbehaving clients to break out of the static files


As far as I understand os.path.realpath will follow symbolic links and here we will stop it from going outside of statics directory.
Instead we can build the path with abspath and not follow symlinks (yet) and prevent breaking outside of directory and if file is a symlink it will do os.stat and by default it will follow symlinks.

Why not change line 165 realpath to abspath? If the directory is a symbolic link, line 166 will fail.

Can we add a test with the scenario @scientificRat mentioned?

sure, I'll take a look.

Not sure if I got the point correctly, but I added the tests for this, but I think this change is not needed.

Add test to prevent path traversal Update requirements.txt Update tests

requirements.txt

tests/test_staticfiles.py

Kludex · 2022-12-12T07:23:06Z

starlette/staticfiles.py

+            full_path = os.path.abspath(os.path.join(directory, path))
            directory = os.path.realpath(directory)
            if os.path.commonprefix([full_path, directory]) != directory:
                # Don't allow misbehaving clients to break out of the static files


Can we add a test with the scenario @scientificRat mentioned?

… into statics-follow-symlinks

…low-symlinks

Kludex

I'm still not 100% sure about this. 😞

I think I need to write a small summary about this, and what are the alternatives if we don't do it. 👀

tests/test_staticfiles.py

Kludex · 2023-02-04T10:21:49Z

My previous concern on this PR was:

If a developer uses the same directory as the StaticFiles to upload files, we can have a scenario on which a symlink is uploaded there, and the machine can be exploited.

Should we be concerned about it? Is this feature wanted that bad? 🤔

Kludex · 2023-02-04T10:22:58Z

Can we make this feature opt-in i.e. add follow_symlink on the StaticFiles?

Let me know what you think @aminalaee

aminalaee · 2023-02-04T12:45:00Z

Can we make this feature opt-in i.e. add follow_symlink on the StaticFiles?

Yeah that works, I will update the PR.

Kludex · 2023-02-04T13:29:46Z

Thanks

…low-symlinks

Kludex · 2023-02-04T16:32:19Z

I guess only docs are missing, and we are ready here 👀

aminalaee · 2023-02-04T16:43:35Z

@Kludex I just updated the docs. Let me know what you think.

docs/staticfiles.md

Kludex · 2023-02-04T17:21:59Z

Thanks @aminalaee :)

Check your Twitter please.

Co-authored-by: Marcelo Trylesinski <[email protected]>

aminalaee mentioned this pull request Jun 10, 2022

Upgrade mkdocs-material to 8.3.3 #1684

Closed

aminalaee marked this pull request as draft June 10, 2022 13:39

Allow StaticFiles follow symlinks

4c6c59e

Add test to prevent path traversal Update requirements.txt Update tests

aminalaee force-pushed the statics-follow-symlinks branch from 9e4d72a to 4c6c59e Compare June 10, 2022 14:28

aminalaee marked this pull request as ready for review June 10, 2022 14:30

aminalaee commented Jun 10, 2022

View reviewed changes

aminalaee requested a review from a team June 10, 2022 14:38

Kludex modified the milestones: Version 0.21.0, Version 0.22.0 Jun 10, 2022

Allow StaticFiles follow symlinks

a799da7

Add test to prevent path traversal Update requirements.txt Update tests

aminalaee force-pushed the statics-follow-symlinks branch from 4c6c59e to a799da7 Compare June 11, 2022 08:02

Kludex added the hold Don't merge it label Jun 11, 2022

yezz123 approved these changes Jun 27, 2022

View reviewed changes

Merge branch 'master' into statics-follow-symlinks

b396b1b

Kludex modified the milestones: Version 0.22.0, Version 0.21.0 Jun 30, 2022

ischaojie reviewed Jul 28, 2022

View reviewed changes

requirements.txt Outdated Show resolved Hide resolved

aminalaee and others added 2 commits July 28, 2022 09:18

Merge branch 'master' into statics-follow-symlinks

db5cd57

Merge branch 'master' into statics-follow-symlinks

ac7d928

Kludex modified the milestones: Version 0.21.0, Version 0.22.0 Sep 24, 2022

Kludex modified the milestones: Version 0.22.0, Version 0.23.0 Nov 20, 2022

Kludex modified the milestones: Version 0.23.0, Version 0.24.0 Dec 8, 2022

Kludex removed the hold Don't merge it label Dec 12, 2022

Kludex requested changes Dec 12, 2022

View reviewed changes

Kludex mentioned this pull request Dec 12, 2022

Version 0.24.0 #1983

Merged

7 tasks

Merge branch 'statics-follow-symlinks' of github.com:encode/starlette…

ebc4f4d

… into statics-follow-symlinks

aminalaee force-pushed the statics-follow-symlinks branch from a54b472 to 9c7eab5 Compare December 12, 2022 10:39

fix naming in tests

140edb6

aminalaee force-pushed the statics-follow-symlinks branch from 9c7eab5 to 140edb6 Compare December 12, 2022 10:39

Merge branch 'master' of github.com:encode/starlette into statics-fol…

5a3c109

…low-symlinks

aminalaee requested a review from Kludex December 12, 2022 10:48

Kludex requested changes Dec 17, 2022

View reviewed changes

tests/test_staticfiles.py Outdated Show resolved Hide resolved

Kludex added the staticfiles Static file serving label Dec 17, 2022

Kludex modified the milestones: Version 0.24.0, Version 0.25.0 Dec 17, 2022

Kludex reviewed Feb 4, 2023

View reviewed changes

tests/test_staticfiles.py Outdated Show resolved Hide resolved

Update tests/test_staticfiles.py

34fe083

Kludex modified the milestones: Version 0.25.0, Version 0.24.0 Feb 4, 2023

aminalaee added 2 commits February 4, 2023 17:29

add follow_symlink

7a8d49c

Merge branch 'master' of github.com:encode/starlette into statics-fol…

c404cb6

…low-symlinks

Kludex reviewed Feb 4, 2023

View reviewed changes

docs/staticfiles.md Show resolved Hide resolved

update docs

e3e85db

aminalaee force-pushed the statics-follow-symlinks branch from f689dc4 to e3e85db Compare February 4, 2023 17:14

Kludex approved these changes Feb 4, 2023

View reviewed changes

Kludex merged commit ea2e794 into master Feb 4, 2023

Kludex deleted the statics-follow-symlinks branch February 4, 2023 17:22

aminalaee added a commit that referenced this pull request Feb 13, 2023

Allow StaticFiles follow symlinks (#1683)

d017340

Co-authored-by: Marcelo Trylesinski <[email protected]>

sync-by-unito bot mentioned this pull request Aug 28, 2023

Bump starlette from 0.22.0 to 0.25.0 toddbirchard/ghost-webhook-api#294

Closed

Uh oh!

Allow StaticFiles follow symlinks #1683

Allow StaticFiles follow symlinks #1683

Uh oh!

Conversation

aminalaee commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aminalaee Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

scientificRat Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

Kludex Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

aminalaee Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

aminalaee Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kludex Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

Kludex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kludex commented Feb 4, 2023

Uh oh!

Kludex commented Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aminalaee commented Feb 4, 2023

Uh oh!

Kludex commented Feb 4, 2023

Uh oh!

Kludex commented Feb 4, 2023

Uh oh!

aminalaee commented Feb 4, 2023

Uh oh!

Uh oh!

Kludex commented Feb 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aminalaee commented Jun 10, 2022 •

edited

Loading

Kludex commented Feb 4, 2023 •

edited

Loading