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

Rule for wildcard CORS in FastAPI #3137

Merged
merged 12 commits into from
Dec 13, 2023
Merged

Rule for wildcard CORS in FastAPI #3137

merged 12 commits into from
Dec 13, 2023

Conversation

theinfosecguy
Copy link
Contributor

FastAPI supports adding middleware using app.add_middleware(). Developers usually add a wildcard origin policy, that can lead to security risks.

@inkz
Copy link
Member

inkz commented Oct 9, 2023

@theinfosecguy

  • Please add
subcategory:
- vuln

like in https://github.com/returntocorp/semgrep-rules/blob/develop/python/flask/security/injection/nan-injection.yaml#L38

  • and make sure that all ruleids are in the right place
  • also please add an ok test case with a non-vulnerable example

@theinfosecguy
Copy link
Contributor Author

Thanks for the feedback, @inkz. I've updated the rule.

Copy link
Member

@inkz inkz left a comment

Choose a reason for hiding this comment

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

Change code of the test to the following (I cant edit your branch):

from fastapi import FastAPI
from fastapi.middleware.cors import CORSMiddleware

app = FastAPI()

origins = ["*"]


app.add_middleware(
    CORSMiddleware,
    # rule-id: wildcard-cors
    allow_origins=origins,
    allow_credentials=True,
    allow=["*"]
)


app.add_middleware(
    CORSMiddleware,
    # rule-id: wildcard-cors
    allow_origins=["*"],
    allow_credentials=True,
    allow=["*"]
)


app.add_middleware(
    CORSMiddleware,
    # ok-id: wildcard-cors
    allow_origins=["https://github.com"],
    allow_credentials=True,
    allow=["*"]
)

app.add_middleware(
    CORSMiddleware,
    # ok: wildcard-cors
    allow_origins=["https://github.com"],
    allow_credentials=True,
    allow=["www.semgrep.dev"]
)


@app.get("/")
async def main():
    return {"message": "Hello Semgrep"}

@theinfosecguy
Copy link
Contributor Author

@inkz It's done.

@inkz inkz self-requested a review December 13, 2023 10:20
@inkz
Copy link
Member

inkz commented Dec 13, 2023

@inkz It's done.

@theinfosecguy
Ok my bad, was too quick to copy paste 😆
can you update it to:

from fastapi import FastAPI
from fastapi.middleware.cors import CORSMiddleware

app = FastAPI()

origins = ["*"]


app.add_middleware(
    CORSMiddleware,
    # ruleid: wildcard-cors
    allow_origins=origins,
    allow_credentials=True,
    allow=["*"]
)


app.add_middleware(
    CORSMiddleware,
    # ruleid: wildcard-cors
    allow_origins=["*"],
    allow_credentials=True,
    allow=["*"]
)


app.add_middleware(
    CORSMiddleware,
    # ok: wildcard-cors
    allow_origins=["https://github.com"],
    allow_credentials=True,
    allow=["*"]
)

app.add_middleware(
    CORSMiddleware,
    # ok: wildcard-cors
    allow_origins=["https://github.com"],
    allow_credentials=True,
    allow=["www.semgrep.dev"]
)


@app.get("/")
async def main():
    return {"message": "Hello Semgrep"}

should work this time 😄

there must be ruleid comments, not rule-ids

@theinfosecguy
Copy link
Contributor Author

hahah, my bad. 😆

Should work now.

@inkz inkz enabled auto-merge (squash) December 13, 2023 11:01
@inkz
Copy link
Member

inkz commented Dec 13, 2023

ok now lgtm! thank you!

@inkz inkz merged commit 65db589 into semgrep:develop Dec 13, 2023
7 checks passed
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.

3 participants