Skip to content

test: add test for PresignedPostPolicy with empty fileName #2119

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
Merged
merged 4 commits into from
Jun 23, 2025
Merged

test: add test for PresignedPostPolicy with empty fileName #2119

merged 4 commits into from
Jun 23, 2025

Conversation

jiuker
Copy link
Contributor

@jiuker jiuker commented Jun 4, 2025

test: add test for PresignedPostPolicy with empty fileName
fix minio/minio#21323 (review)

We should add a test for this in mint.

test: add test for PresignedPostPolicy with empty fileName
fed4256 
test: add test for PresignedPostPolicy with empty fileName
Copilot
Copilot AI reviewed Jun 4, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a new functional test to verify behavior when the form file name is empty in a presigned POST policy, and wires it into the test suite.

  • Introduce testPresignedPostPolicyEmptyFileName() covering empty filename error case
  • Invoke the new test in main()
  • No other functional changes

functional_tests.go
logError(testName, function, args, startTime, "", "File open failed", err)
return
}
w, err := writer.CreateFormFile("", filePath)
Copy link
Preview

Copilot AI Jun 4, 2025

Choose a reason for hiding this comment

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

The arguments to CreateFormFile are swapped: the first parameter is the form field name and the second is the filename. To test an empty filename, use writer.CreateFormFile("file", "") (or swap the args) instead of an empty field name.

Copilot uses AI. Check for mistakes.

functional_tests.go
@@ -5586,6 +5586,161 @@ func testPresignedPostPolicyWrongFile() {
logSuccess(testName, function, args, startTime)
}

// testPresignedPostPolicyEmptyFileName tests that an empty file name in the presigned post policy
func testPresignedPostPolicyEmptyFileName() {
Copy link
Preview

Copilot AI Jun 4, 2025

Choose a reason for hiding this comment

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

[nitpick] This new test duplicates a large portion of testPresignedPostPolicy and testPresignedPostPolicyWrongFile. Consider extracting common setup (client initialization, bucket creation, data reader, policy builder) into a helper to reduce duplication and improve readability.

Copilot uses AI. Check for mistakes.

@jiuker
Copy link
Contributor Author

jiuker commented Jun 4, 2025 •
edited
Loading

@harshavardhana Looks like need the latest minio which have the fix to test https://github.com/minio/minio-go/actions/runs/15437786953/job/43448139531?pr=2119

{"time":"2025-06-04T08:49:05.758503756Z","level":"INFO","name":"minio-go: testPresignedPostPolicy","duration":22,"function":"PresignedPostPolicy(policy)","args":{"policy":"{\"expiration\":\"2025-06-14T08:49:05.743Z\",\"conditions\":[[\"eq\",\"$bucket\",\"minio-go-test-lbrqmi4xsygunklr\"],[\"eq\",\"$key\",\"sb4h6ajpod9bijx1c60olk1rqylcwv\"],[\"eq\",\"$Content-Type\",\"binary/octet-stream\"],[\"eq\",\"$x-amz-meta-userrwz5oksy0daruerus9cz3qdlgk\",\"p2ihru65wooasbucen69enghxvz6sa\"],[\"eq\",\"$Content-Encoding\",\"gzip\"],[\"eq\",\"$x-amz-checksum-algorithm\",\"CRC32C\"],[\"eq\",\"$x-amz-checksum-crc32c\",\"aHnJMw==\"],[\"content-length-range\", 10, 1048576]]}","url":"https://localhost:9000/minio-go-test-lbrqmi4xsygunklr/"},"status":"PASS"}
{"time":"2025-06-04T08:49:05.778829608Z","level":"INFO","name":"minio-go: testPresignedPostPolicyWrongFile","duration":13,"function":"PresignedPostPolicy(policy)","args":{"policy":"{\"expiration\":\"2025-06-14T08:49:05.772Z\",\"conditions\":[[\"eq\",\"$bucket\",\"minio-go-test-w4ombnn9vsb5ychf\"],[\"eq\",\"$key\",\"eplpx1b9910iy51jnb155nvwfxs5a9\"],[\"eq\",\"$Content-Type\",\"binary/octet-stream\"],[\"eq\",\"$x-amz-meta-user422zdocvnpvq0xeqa2tzzt02r0\",\"6ipbcbif32txypqrwdxxkrvvcnxs9t\"],[\"eq\",\"$x-amz-checksum-algorithm\",\"CRC32C\"],[\"eq\",\"$x-amz-checksum-crc32c\",\"8TDyHg==\"],[\"content-length-range\", 10, 1048576]]}","url":"https://localhost:9000/minio-go-test-w4ombnn9vsb5ychf/"},"status":"PASS"}
ERRO: panic: "POST /minio-go-test-eccrd4n9lvsokpql/": runtime error: invalid memory address or nil pointer dereference
goroutine 150256 [running]:
runtime/debug.Stack()
	runtime/debug/stack.go:26 +0x5e
github.com/minio/minio/cmd.serverMain.func10.setCriticalErrorHandler.2.1()
	github.com/minio/minio/cmd/generic-handlers.go:583 +0x99
panic({0x306c540?, 0x6cfa2e0?})
	runtime/panic.go:792 +0x132
github.com/minio/minio/internal/etag.(*Reader).Read(0xc002fc14c0, {0xc00d6ba000, 0x43b987?, 0x200000})
	github.com/minio/minio/internal/etag/reader.go:129 +0x36
github.com/minio/minio/internal/hash.(*Reader).Read(0xc010c0e0d0, {0xc00d6ba000, 0xc00d343f78?, 0x200000})
	github.com/minio/minio/internal/hash/reader.go:251 +0x42
io.ReadAtLeast({0x4f10500, 0xc010c0e0d0}, {0xc00d6ba000, 0x100000, 0x200000}, 0x100000)
	io/io.go:335 +0x91
io.ReadFull(...)

@klauspost
Copy link
Contributor

@jiuker What you posted looks more like a bug than a test failure to me. Consider failing rather than crashing.

@jiuker
Copy link
Contributor Author

jiuker commented Jun 12, 2025 •
edited
Loading

@jiuker What you posted looks more like a bug than a test failure to me. Consider failing rather than crashing.

A bug fixed at minio main branch.

@klauspost
Copy link
Contributor

@jiuker Which one?

@jiuker
Copy link
Contributor Author

jiuker commented Jun 13, 2025

@jiuker Which one?

minio/minio#21323 this one

@klauspost
Copy link
Contributor

@jiuker How can a remote package cause a runtime error: invalid memory address or nil pointer dereference in a local test?

@jiuker
Copy link
Contributor Author

jiuker commented Jun 13, 2025

@jiuker How can a remote package cause a runtime error: invalid memory address or nil pointer dereference in a local test?

Should be remote minio have the bug code.

@klauspost
Copy link
Contributor

@jiuker The test in this package should not panic, just because it doesn't get the response it expects. It should log the error and move on.

@jiuker
Copy link
Contributor Author

jiuker commented Jun 17, 2025

@jiuker The test in this package should not panic, just because it doesn't get the response it expects. It should log the error and move on._这个包中_的测试不应该仅仅因为没有得到预期的响应而死机。它应该记录错误并继续前进。

Panic happend at minio code, not this package.

@harshavardhana
Copy link
Member

Panic happend at minio code, not this package.

are you sending a fix?

@jiuker
Copy link
Contributor Author

jiuker commented Jun 17, 2025 •
edited
Loading

Panic happend at minio code, not this package.

are you sending a fix?

Already merged minio/minio#21323 @harshavardhana

minio-go/.github/workflows/go.yml

Lines 43 to 51 in 1744ff2

run: |
sudo apt update -y
sudo apt install devscripts -y
wget -O /tmp/minio https://dl.minio.io/server/minio/release/linux-amd64/minio
chmod +x /tmp/minio
mkdir -p /tmp/certs-dir
cp testcerts/* /tmp/certs-dir
/tmp/minio server --quiet -S /tmp/certs-dir /tmp/fs{1...4} &
make

This release don't have that fix

@harshavardhana harshavardhana merged commit 8eacd80 into minio:master Jun 23, 2025
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@harshavardhana harshavardhana harshavardhana approved these changes

@klauspost klauspost Awaiting requested review from klauspost

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jiuker @klauspost @harshavardhana