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.

Sign up for GitHub

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

Jump to bottom

test(middleware/csrf): Fix Benchmark Tests #2932

Merged
merged 2 commits into from
Mar 25, 2024
Merged

test(middleware/csrf): Fix Benchmark Tests #2932

merged 2 commits into from
Mar 25, 2024

Conversation

sixcolors
Copy link
Member

@sixcolors sixcolors commented Mar 25, 2024 •
edited
Loading

PR Description:

This PR addresses a bug in the CSRF middleware test suite and enhances the referer header validation logic for improved efficiency. Here's a summary of the changes:

Changes Made:

  • Bug Fix:

    • Fixed a bug in the benchmark tests of csrf_test.go where headers were not properly set for POST requests, causing incorrect test results. The fix ensures that the referer header is correctly set with different schemes and hosts, covering various scenarios.
    • Fixed a bug in the benchmark tests of csrf_test.go where there were two contexts and only one had anything set, causing all requests to be made with an empty context.

  • Code Logic Enhancement:

    • Refactored the logic to convert the referer header to lowercase before comparison, ensuring consistent handling of the header case.
    • Simplified the comparison logic by directly comparing the scheme and host components of the referer URL with those of the request, eliminating unnecessary string concatenation.

Notes for Review:

@sixcolors sixcolors requested a review from a team as a code owner March 25, 2024 00:55
@sixcolors sixcolors requested review from gaby, ReneWerner87 and efectn and removed request for a team March 25, 2024 00:55
@sixcolors sixcolors changed the base branch from main to v2 March 25, 2024 00:55
@sixcolors
Copy link
Member Author

@coderabbitai stop your review of PR with a target of main, I changed the taget to the v2 branch.

@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 25, 2024

@sixcolors, I've noted your request to stop the review of the PR that was targeting the main branch, as you've changed the target to the v2 branch. The review process will be paused accordingly.

Actions performed

Reviews paused.

@sixcolors
Copy link
Member Author

@coderabbitai review

coderabbitai[bot]
coderabbitai bot reviewed Mar 25, 2024
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 54eae2a and ec6c81d.
Files selected for processing (1)
  • middleware/csrf/csrf.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
  • middleware/csrf/csrf.go

@gofiber gofiber deleted a comment from coderabbitai bot Mar 25, 2024
@ReneWerner87 ReneWerner87 added this to the Next Release milestone Mar 25, 2024
@ReneWerner87 ReneWerner87 merged commit ba10e68 into gofiber:v2 Mar 25, 2024
20 checks passed
@sixcolors sixcolors deleted the fix-csrf-benchmark branch March 27, 2024 01:17
ReneWerner87 added a commit that referenced this pull request Mar 28, 2024
@ReneWerner87 @tokelo-12
@sixcolors @dependabot @iredmail @benjajaja @MehmetFiratKomurcu @brunodmartins @muhammadkholidb @gilwo @luk3skyw4lker @efectn @gaby @CAEL0 @grivera64 @coderabbitai
Main v2 merge (#2944)
60967f5 
* Update pull_request_template.md

* Update v3-changes.md

* Update CONTRIBUTING.md (#2752)

Grammar correction.

* chore(encryptcookie)!: update default config (#2753)

* chore(encryptcookie)!: update default config

docs(encryptcookie): enhance documentation and examples

BREAKING CHANGE: removed the hardcoded "csrf_" from the Except.

* docs(encryptcookie): reads or modifies cookies

* chore(encryptcookie): csrf config example

* docs(encryptcookie): md table spacing

* build(deps): bump actions/setup-go from 4 to 5 (#2754)

Bumps [actions/setup-go](https://github.com/actions/setup-go) from 4 to 5.
- [Release notes](https://github.com/actions/setup-go/releases)
- [Commits](actions/setup-go@v4...v5)

---
updated-dependencies:
- dependency-name: actions/setup-go
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* 🩹 middleware/logger/: log client IP address by default (#2755)

* middleware/logger: Log client IP address by default.

* Update doc.

* fix: don't constrain middlewares' context-keys to strings 🐛 (#2751)

* Revert "Revert ":bug: requestid.Config.ContextKey is interface{} (#2369)" (#2742)"

This reverts commit 28be17f.

* fix: request ContextKey default value condition

Should check for `nil` since it is `any`.

* fix: don't constrain middlewares' context-keys to strings

`context` recommends using "unexported type" as context keys to avoid
collisions https://pkg.go.dev/github.com/gofiber/fiber/v2#Ctx.Locals.

The official go blog also recommends this https://go.dev/blog/context.

`fiber.Ctx.Locals(key any, value any)` correctly allows consumers to
use unexported types or e.g. strings.

But some fiber middlewares constrain their context-keys to `string` in
their "default config structs", making it impossible to use unexported
types.

This PR removes the `string` _constraint_ from all middlewares, allowing
to now use unexported types as per the official guidelines. However
the default value is still a string, so it's not a breaking change, and
anyone still using strings as context keys is not affected.

* 📚 Update app.md for indentation (#2761)

Update app.md for indentation

* build(deps): bump github.com/google/uuid from 1.4.0 to 1.5.0 (#2762)

Bumps [github.com/google/uuid](https://github.com/google/uuid) from 1.4.0 to 1.5.0.
- [Release notes](https://github.com/google/uuid/releases)
- [Changelog](https://github.com/google/uuid/blob/master/CHANGELOG.md)
- [Commits](google/uuid@v1.4.0...v1.5.0)

---
updated-dependencies:
- dependency-name: github.com/google/uuid
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump github/codeql-action from 2 to 3 (#2763)

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2 to 3.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@v2...v3)

---
updated-dependencies:
- dependency-name: github/codeql-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Changing default log output (#2730)

changing default log output

Closes #2729

* Update hooks.md

fix wrong hooks signature

* 🩹 Fix: CORS middleware should use the defined AllowedOriginsFunc config when AllowedOrigins is empty (#2771)

* 🐛 [Bug]: Adaptator + otelfiber issue #2641 (#2772)

* 🩹🚨 - fix for redirect with query params (#2748)

* redirect with query params did not work, fix it and add test for it

* redirect middleware - fix test typo

* ♻️ logger/middleware colorize logger error message #2593 (#2773)

* ✨ feat: add liveness and readiness checks (#2509)

* ✨ feat: add liveness and readiness checkers

* 📝 docs: add docs for liveness and readiness

* ✨ feat: add options method for probe checkers

* ✅ tests: add tests for liveness and readiness

* ♻️ refactor: change default endpoint values

* ♻️ refactor: change default value for liveness endpoint

* 📝 docs: add return status for liveness and readiness probes

* ♻️ refactor: change probechecker to middleware

* 📝 docs: move docs to middleware session

* ♻️ refactor: apply gofumpt formatting

* ♻️ refactor: remove unused parameter

* split config and apply a review

* apply reviews and add testcases

* add benchmark

* cleanup

* rename middleware

* fix linter

* Update docs and config values

* Revert change to IsReady

* Updates based on code review

* Update docs to match other middlewares

---------

Co-authored-by: Muhammed Efe Cetin <efectn@protonmail.com>
Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com>
Co-authored-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com>

* prepare release v2.52.0
- add more Parser tests

* fix healthcheck.md

* configure workflows for V2 branch

* configure workflows for V2 branch

* Fix default value to false in docs of QueryBool (#2811)

fix default value to false in docs of QueryBool

* update queryParser config

* Update ctx.md

* Update routing.md

* 📚 Doc: Fix code snippet indentation in /docs/api/middleware/keyauth.md

Removes an an extra level of indentation in line 51 of
`keyauth.md` [here](https://github.com/gofiber/fiber/blob/v2/docs/api/middleware/keyauth.md?plain=1#L51)

* fix: healthcheck middleware not working with route group (#2863)

* fix: healthcheck middleware not working with route group

* perf: change verification method to improve perf

* Update healthcheck_test.go

* test: add not matching route test for strict routing

* add more test cases

* correct tests

* correct test helpers

* correct tests

* correct tests

---------

Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com>
Co-authored-by: René Werner <rene@gofiber.io>

* Merge pull request from GHSA-fmg4-x8pw-hjhg

* Enforce Wildcard Origins with AllowCredentials check

* Expand unit-tests, fix issues with subdomains logic, update docs

* Update cors.md

* Added test using localhost, ipv4, and ipv6 address

* improve documentation markdown

---------

Co-authored-by: René Werner <rene@gofiber.io>

* Update app.go

prepare release v2.52.1

* fix cors domain normalize

* fix sync-docs workflow

* fix sync-docs workflow

* fix(middleware/cors): Validation of multiple Origins (#2883)

* fix: allow origins check

Refactor CORS origin validation and normalization to trim leading or trailing whitespace in the cfg.AllowOrigins string [list]. URLs with whitespace inside the URL are invalid, so the normalizeOrigin will return false because url.Parse will fail, and the middleware will panic.

fixes #2882

* test: AllowOrigins with whitespace

* test(middleware/cors): add benchmarks

* chore: fix linter errors

* test(middleware/cors): use h() instead of app.Test()

* test(middleware/cors): add miltiple origins in Test_CORS_AllowOriginScheme

* chore: refactor validate and normalize

* test(cors/middleware): add more benchmarks

* prepare release v2.52.2

* refactor(docs): deactivate docs sync for v2

* refactor(docs): deactivate docs sync for v2

* fix(middleware/cors): Handling and wildcard subdomain matching (#2915)

* fix: allow origins check

Refactor CORS origin validation and normalization to trim leading or trailing whitespace in the cfg.AllowOrigins string [list]. URLs with whitespace inside the URL are invalid, so the normalizeOrigin will return false because url.Parse will fail, and the middleware will panic.

fixes #2882

* test: AllowOrigins with whitespace

* test(middleware/cors): add benchmarks

* chore: fix linter errors

* test(middleware/cors): use h() instead of app.Test()

* test(middleware/cors): add miltiple origins in Test_CORS_AllowOriginScheme

* chore: refactor validate and normalize

* test(cors/middleware): add more benchmarks

* fix(middleware/cors): handling and wildcard subdomain matching

docs(middleware/cors): add How it works and Security Considerations

* chore: grammar

* Apply suggestions from code review

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* chore: fix misspelling

* test(middleware/cors): combine Invalid_Origins tests

* refactor(middleware/cors): headers handling

* docs(middleware/cors): Update AllowOrigins description

* chore: merge

* perf(middleware/cors): optimize handler

* perf(middleware/cors): optimize handler

* chore(middleware/cors): ipdate origin handling logic

* chore(middleware/cors): fix header capitalization

* docs(middleware/cors): improve sercuity notes

* docs(middleware/cors): Improve security notes

* docs(middleware/cors): improve CORS overview

* docs(middleware/cors): fix ordering of how it works

* docs(middleware/cors): add additional info to How to works

* docs(middleware/cors): rm space

* docs(middleware/cors): add validation for AllowOrigins origins to overview

* docs(middleware/cors): update ExposeHeaders and MaxAge descriptions

* docs(middleware/cors): Add dynamic origin validation example

* docs(middleware/cors): Improve security notes and fix header capitalization

* docs(middleware/cors): configuration examples

* docs(middleware/cors): `"*"`

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* fix(middleware/cors): Categorize requests correctly (#2921)

* fix(middleware/cors): categorise requests correctly

* test(middleware/cors): improve test coverage for request types

* test(middleware/cors): Add subdomain matching tests

* test(middleware/cors): parallel tests for CORS headers based on request type

* test(middleware/cors): Add benchmark for CORS subdomain matching

* test(middleware/cors): cover additiona test cases

* refactor(middleware/cors): origin validation and normalization

* test(middleware/csrf): Fix Benchmark Tests (#2932)

* test(middleware/csrf): fix Benchmark_Middleware_CSRF_*

* fix(middleware/csrf): update refererMatchesHost()

* Prepare release v2.52.3

* fix(middleware/cors): CORS handling (#2937)

* fix(middleware/cors): CORS handling

* fix(middleware/cors): Vary header handling

* test(middleware/cors): Ensure Vary Headers checked

* fix(middleware/cors): Vary header handling non-cors OPTIONS requests (#2939)

* fix(middleware/cors): Vary header handling non-cors OPTIONS requests

* chore(middleware/cors): Add Vary header for non-CORS OPTIONS requests comment

* prepare release v2.52.4

* merge v2 in main(v3)

* merge v2 in main(v3)

* merge v2 in main(v3)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: tokelo-12 <113810058+tokelo-12@users.noreply.github.com>
Co-authored-by: Jason McNeil <sixcolors@mac.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: iRedMail <2048991+iredmail@users.noreply.github.com>
Co-authored-by: Benjamin Grosse <ste3ls@gmail.com>
Co-authored-by: Mehmet Firat KOMURCU <mehmetfiratkomurcu@hotmail.com>
Co-authored-by: Bruno <bdm2943@icloud.com>
Co-authored-by: Muhammad Kholid B <muhammadkholidb@gmail.com>
Co-authored-by: gilwo <gilwo@users.noreply.github.com>
Co-authored-by: Lucas Lemos <lucashenriqueblemos@gmail.com>
Co-authored-by: Muhammed Efe Cetin <efectn@protonmail.com>
Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com>
Co-authored-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com>
Co-authored-by: Jongmin Kim <kjongmin26@gmail.com>
Co-authored-by: Giovanni Rivera <rivera.giovanni271@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
fsandel pushed a commit to stackitcloud/stackit-cert-manager-webhook that referenced this pull request Apr 4, 2024
Merged PR 549976: Update module github.com/gofiber/fiber/v2 to v2.52.4
1ecf8d1 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/gofiber/fiber/v2](https://github.com/gofiber/fiber) | require | patch | `v2.52.2` -> `v2.52.4` |

---

### Release Notes

<details>
<summary>gofiber/fiber (github.com/gofiber/fiber/v2)</summary>

### [`v2.52.4`](https://github.com/gofiber/fiber/releases/tag/v2.52.4)

[Compare Source](gofiber/fiber@v2.52.3...v2.52.4)

### 🐛 Fixes

-   Middleware/cors: CORS handling by [@&#8203;sixcolors](https://github.com/sixcolors) in gofiber/fiber#2937
-   Middleware/cors: Vary header handling non-cors OPTIONS requests by [@&#8203;sixcolors](https://github.com/sixcolors) in gofiber/fiber#2939

**Full Changelog**: gofiber/fiber@v2.52.3...v2.52.4

### [`v2.52.3`](https://github.com/gofiber/fiber/releases/tag/v2.52.3)

[Compare Source](gofiber/fiber@v2.52.2...v2.52.3)

#### 🐛 Fixes

-   Middleware/cors: Handling and wildcard subdomain matching by [@&#8203;sixcolors](https://github.com/sixcolors) in gofiber/fiber#2915
-   Middleware/cors: Categorize requests correctly by [@&#8203;sixcolors](https://github.com/sixcolors) in gofiber/fiber#2921
-   Middleware/csrf: Fix Benchmark Tests by [@&#8203;sixcolors](https://github.com/sixcolors) in gofiber/fiber#2932

**Full Changelog**: gofiber/fiber@v2.52.2...v2.52.3

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or rename PR to start with "rebase!".

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@gaby gaby gaby approved these changes

@ReneWerner87 ReneWerner87 Awaiting requested review from ReneWerner87 ReneWerner87 is a code owner automatically assigned from gofiber/maintainers

@efectn efectn Awaiting requested review from efectn efectn is a code owner automatically assigned from gofiber/maintainers

Assignees
No one assigned
Labels
☢️ Bug 🧹 Updates v2
Projects
None yet
Milestone
v2.52.3
Development

Successfully merging this pull request may close these issues.
3 participants
@sixcolors @gaby @ReneWerner87