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

fix CSRF middleware not being able to extract token from multipart/form-data form #2136

Merged
merged 1 commit into from Mar 15, 2022

Conversation

aldas
Copy link
Contributor

@aldas aldas commented Mar 15, 2022

Fix for #2135 CSRF middleware not being able to extract token from multipart/form-data form

This can be tested by hand:

func main() {
	e := echo.New()
	e.Use(middleware.Logger())
	e.Use(middleware.Recover())

	e.Use(middleware.CSRFWithConfig(middleware.CSRFConfig{
		TokenLookup: "form:csrf",
	}))

	e.POST("/form", func(c echo.Context) error {
		csrf, ok := c.Get("csrf").(string)
		if !ok {
			return echo.NewHTTPError(http.StatusBadRequest, "missing CSRF value")
		}
		return c.String(http.StatusCreated, csrf)
	})

	if err := e.Start(":8080"); err != http.ErrServerClosed {
		log.Fatal(err)
	}
}
echo "<div>hi</div>" > test.html

curl --trace-ascii /dev/stdout \
    --cookie "_csrf=test" \
    -F csrf=test \
    -F upload=@test.html \
    http://localhost:8080/form

output of curl after fix

x@x:~/code/echo/main$ curl --trace-ascii /dev/stdout --cookie "_csrf=test" -F csrf=test -F upload=@test.html http://localhost:8080/form
== Info:   Trying 127.0.0.1:8080...
== Info: Connected to localhost (127.0.0.1) port 8080 (#0)
=> Send header, 210 bytes (0xd2)
0000: POST /form HTTP/1.1
0015: Host: localhost:8080
002b: User-Agent: curl/7.74.0
0044: Accept: */*
0051: Cookie: _csrf=test
0065: Content-Length: 299
007a: Content-Type: multipart/form-data; boundary=--------------------
00ba: ----a297ba70c335a0d7
00d0: 
=> Send data, 299 bytes (0x12b)
0000: --------------------------a297ba70c335a0d7
002c: Content-Disposition: form-data; name="csrf"
0059: 
005b: test
0061: --------------------------a297ba70c335a0d7
008d: Content-Disposition: form-data; name="upload"; filename="test.ht
00cd: ml"
00d2: Content-Type: text/html
00eb: 
00ed: <div>hi</div>.
00fd: --------------------------a297ba70c335a0d7--
== Info: We are completely uploaded and fine
== Info: Mark bundle as not supporting multiuse
<= Recv header, 22 bytes (0x16)
0000: HTTP/1.1 201 Created
<= Recv header, 41 bytes (0x29)
0000: Content-Type: text/plain; charset=UTF-8
<= Recv header, 63 bytes (0x3f)
0000: Set-Cookie: _csrf=test; Expires=Wed, 16 Mar 2022 19:00:31 GMT
<= Recv header, 14 bytes (0xe)
0000: Vary: Cookie
<= Recv header, 37 bytes (0x25)
0000: Date: Tue, 15 Mar 2022 19:00:31 GMT
<= Recv header, 19 bytes (0x13)
0000: Content-Length: 4
<= Recv header, 2 bytes (0x2)
0000: 
<= Recv data, 4 bytes (0x4)
0000: test
== Info: Connection #0 to host localhost left intact
test

@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #2136 (283d1ac) into master (b445958) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2136   +/-   ##
=======================================
  Coverage   92.10%   92.10%           
=======================================
  Files          37       37           
  Lines        3028     3028           
=======================================
  Hits         2789     2789           
  Misses        150      150           
  Partials       89       89           
Impacted Files Coverage Δ
middleware/extractor.go 98.83% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b445958...283d1ac. Read the comment docs.

@aldas aldas requested a review from lammel March 15, 2022 19:04
@aldas
Copy link
Contributor Author

aldas commented Mar 15, 2022

@lammel please review

lammel
lammel approved these changes Mar 15, 2022
Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

LGTM

@lammel lammel merged commit 01d7d01 into labstack:master Mar 15, 2022
15 checks passed
@aldas aldas deleted the fix_csrf_multipartform_values branch July 12, 2022 19:04
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.

None yet

2 participants