Merge pull request #469 from fredbi/fix/3113-file-urlencoded

Fix/3113 file urlencoded

Author: fredbi

.codecov.yml

@@ -0,0 +1,9 @@
+codecov:
+  notify:
+    after_n_builds: 2
+
+coverage:
+  status:
+    patch:
+      default:
+        target: 80%

.github/workflows/skipauth-test.yml

@@ -0,0 +1,51 @@
+name: unsafe-skipauth test
+
+permissions:
+  pull-requests: read
+  contents: read
+
+on:
+  push:
+    branches:
+      - master
+
+  pull_request:
+
+jobs:
+  unsafe-skipauth-test:
+    runs-on: ubuntu-latest
+
+    strategy:
+      matrix:
+        go-version: [stable, oldstable]
+
+    steps:
+      -
+        name: Checkout
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+      -
+        name: Setup Go
+        uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0
+        with:
+          go-version: ${{ matrix.go-version }}
+      -
+        name: Test the dev-mode SetSkipAuth path
+        run: >
+          go test -tags openapi_unsafe_skipauth -race -count=1
+          -coverprofile='skipauth.coverage.ubuntu-latest-${{ matrix.go-version }}.out'
+          -covermode=atomic
+          -coverpkg='github.com/go-openapi/runtime/middleware'
+          ./middleware/...
+      -
+        name: Upload coverage artifacts
+        uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+        with:
+          path: 'skipauth.coverage.ubuntu-latest-${{ matrix.go-version }}.out'
+          name: 'skipauth.coverage.ubuntu-latest-${{ matrix.go-version }}'
+          retention-days: 1
+
+  collect-coverage:
+    needs: [unsafe-skipauth-test]
+    if: ${{ !cancelled() && needs.unsafe-skipauth-test.result == 'success' }}
+    uses: go-openapi/ci-workflows/.github/workflows/collect-coverage.yml@7982843ba86a9b47f456a44ed94e82f882b50a8d # v0.2.17
+    secrets: inherit

client/mock_test.go

@@ -22,6 +22,32 @@ func (m *mockRuntime) Submit(operation *runtime.ClientOperation) (any, error) {
 	return map[string]any{}, nil
 }
 
+// mockContextualRuntime satisfies [ContextualTransport] and records
+// which entry point a caller took plus the context observed at each.
+// Used to verify that wrappers prefer SubmitContext when available
+// and fall back to Submit otherwise.
+type mockContextualRuntime struct {
+	mockRuntime
+
+	submitCalls        int
+	submitContextCalls int
+	lastSubmitCtx      context.Context //nolint:containedctx // test-only inspection of the ctx forwarded by the wrapper
+	lastOpCtx          context.Context //nolint:containedctx // test-only inspection of op.Context as seen by the wrapped transport
+}
+
+func (m *mockContextualRuntime) Submit(operation *runtime.ClientOperation) (any, error) {
+	m.submitCalls++
+	m.lastOpCtx = operation.Context
+	return m.mockRuntime.Submit(operation)
+}
+
+func (m *mockContextualRuntime) SubmitContext(ctx context.Context, operation *runtime.ClientOperation) (any, error) {
+	m.submitContextCalls++
+	m.lastSubmitCtx = ctx
+	m.lastOpCtx = operation.Context
+	return m.mockRuntime.Submit(operation)
+}
+
 type tres struct {
 }
 

client/opentelemetry.go

@@ -4,6 +4,7 @@
 package client
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"strings"
@@ -26,9 +27,14 @@ const (
 
 // WithOpenTelemetry adds opentelemetry support to the provided runtime.
 // A new client span is created for each request.
-// If the context of the client operation does not contain an active span, no span is created.
 // The provided opts are applied to each spans - for example to add global tags.
-func (r *Runtime) WithOpenTelemetry(opts ...OpenTelemetryOpt) runtime.ClientTransport {
+//
+// The returned transport satisfies [ContextualTransport]: callers
+// should prefer [openTelemetryTransport.SubmitContext] over the
+// legacy [runtime.ClientOperation.Context] field. Setting that
+// field is still honored on the [openTelemetryTransport.Submit]
+// compatibility path.
+func (r *Runtime) WithOpenTelemetry(opts ...OpenTelemetryOpt) ContextualTransport {
 	return newOpenTelemetryTransport(r, r.Host, opts)
 }
 
@@ -52,7 +58,7 @@ func (r *Runtime) WithOpenTelemetry(opts ...OpenTelemetryOpt) runtime.ClientTran
 // usual opentracing options and opentracing-enabled transport.
 //
 // Passed options are ignored unless they are of type [OpenTelemetryOpt].
-func (r *Runtime) WithOpenTracing(opts ...any) runtime.ClientTransport {
+func (r *Runtime) WithOpenTracing(opts ...any) ContextualTransport {
 	otelOpts := make([]OpenTelemetryOpt, 0, len(opts))
 	for _, o := range opts {
 		otelOpt, ok := o.(OpenTelemetryOpt)
@@ -155,11 +161,31 @@ func newOpenTelemetryTransport(transport runtime.ClientTransport, host string, o
 	return tr
 }
 
+// Submit implements [runtime.ClientTransport]. It honors the legacy
+// [runtime.ClientOperation.Context] field for backward compatibility
+// — that field is being phased out; new code should call
+// [openTelemetryTransport.SubmitContext] directly with an explicit
+// context.
 func (t *openTelemetryTransport) Submit(op *runtime.ClientOperation) (any, error) {
-	if op.Context == nil {
-		return t.transport.Submit(op)
+	ctx := op.Context
+	if ctx == nil {
+		ctx = context.Background()
 	}
+	return t.SubmitContext(ctx, op)
+}
 
+// SubmitContext submits an operation with an explicit context that
+// drives both the tracing span and (when supported) the wrapped
+// transport's SubmitContext call. The legacy
+// [runtime.ClientOperation.Context] field is not consulted.
+//
+// When the wrapped transport implements [ContextualTransport], ctx is
+// forwarded directly via its SubmitContext. Otherwise, the legacy
+// Submit path is used: ctx is stamped onto op.Context for the
+// duration of that call and restored afterwards, so the wrapped
+// transport still receives a usable context. The legacy fallback
+// disappears once SubmitContext is universal (v2).
+func (t *openTelemetryTransport) SubmitContext(ctx context.Context, op *runtime.ClientOperation) (any, error) {
 	params := op.Params
 	reader := op.Reader
 
@@ -171,7 +197,7 @@ func (t *openTelemetryTransport) Submit(op *runtime.ClientOperation) (any, error
 	}()
 
 	op.Params = runtime.ClientRequestWriterFunc(func(req runtime.ClientRequest, reg strfmt.Registry) error {
-		span = t.newOpenTelemetrySpan(op, req.GetHeaderParams())
+		span = t.newOpenTelemetrySpan(ctx, op, req.GetHeaderParams())
 		return params.WriteToRequest(req, reg)
 	})
 
@@ -191,7 +217,7 @@ func (t *openTelemetryTransport) Submit(op *runtime.ClientOperation) (any, error
 		return reader.ReadResponse(response, consumer)
 	})
 
-	submit, err := t.transport.Submit(op)
+	submit, err := t.submitWrapped(ctx, op)
 	if err != nil && span != nil {
 		span.RecordError(err)
 		span.SetStatus(codes.Error, err.Error())
@@ -200,9 +226,18 @@ func (t *openTelemetryTransport) Submit(op *runtime.ClientOperation) (any, error
 	return submit, err
 }
 
-func (t *openTelemetryTransport) newOpenTelemetrySpan(op *runtime.ClientOperation, header http.Header) trace.Span {
-	ctx := op.Context
+//nolint:contextcheck // ctx is forwarded verbatim; the legacy Submit branch only stamps it onto op.Context for the wrapped transport.
+func (t *openTelemetryTransport) submitWrapped(ctx context.Context, op *runtime.ClientOperation) (any, error) {
+	if sc, ok := t.transport.(ContextualTransport); ok {
+		return sc.SubmitContext(ctx, op)
+	}
+	prev := op.Context
+	op.Context = ctx
+	defer func() { op.Context = prev }()
+	return t.transport.Submit(op)
+}
 
+func (t *openTelemetryTransport) newOpenTelemetrySpan(ctx context.Context, op *runtime.ClientOperation, header http.Header) trace.Span {
 	tracer := t.tracer
 	if tracer == nil {
 		if span := trace.SpanFromContext(ctx); span.SpanContext().IsValid() {

client/opentelemetry_test.go

@@ -62,6 +62,9 @@ func Test_OpenTelemetryRuntime_submit_nilAuthInfo(t *testing.T) {
 }
 
 func Test_OpenTelemetryRuntime_submit_nilContext(t *testing.T) {
+	// With op.Context nil, Submit now defaults to context.Background()
+	// and still produces a span (under the modern SubmitContext path).
+	// Previously this case skipped tracing entirely.
 	exporter := tracetest.NewInMemoryExporter()
 
 	tp := tracesdk.NewTracerProvider(
@@ -77,7 +80,7 @@ func Test_OpenTelemetryRuntime_submit_nilContext(t *testing.T) {
 	operation := testOperation(ctx)
 	operation.Context = nil
 
-	assertOpenTelemetrySubmit(t, operation, exporter, 0) // just don't panic
+	assertOpenTelemetrySubmit(t, operation, exporter, 1)
 }
 
 func Test_injectOpenTelemetrySpanContext(t *testing.T) {
@@ -106,6 +109,72 @@ func Test_injectOpenTelemetrySpanContext(t *testing.T) {
 	assert.Len(t, header, 1)
 }
 
+// Test_OpenTelemetryRuntime_submitContext exercises the modern
+// SubmitContext entry point with a wrapped transport that satisfies
+// [ContextualTransport]. The wrapper must forward the explicit ctx
+// via the wrapped transport's SubmitContext (not via the legacy
+// op.Context field).
+func Test_OpenTelemetryRuntime_submitContext(t *testing.T) {
+	exporter := tracetest.NewInMemoryExporter()
+	tp := tracesdk.NewTracerProvider(
+		tracesdk.WithSampler(tracesdk.AlwaysSample()),
+		tracesdk.WithSyncer(exporter),
+	)
+	otel.SetTracerProvider(tp)
+
+	tracer := tp.Tracer("go-runtime")
+	ctx, span := tracer.Start(context.Background(), "op")
+	defer span.End()
+
+	header := map[string][]string{}
+	mock := &mockContextualRuntime{mockRuntime: mockRuntime{req: runtime.TestClientRequest{Headers: header}}}
+	tr := newOpenTelemetryTransport(mock, "remote_host", nil)
+
+	// Sentinel in op.Context: the SubmitContext path must ignore
+	// it (no read for tracing, no mutation) and forward `ctx` instead.
+	type sentinelKey struct{}
+	staleCtx := context.WithValue(context.Background(), sentinelKey{}, "stale")
+	op := testOperation(staleCtx)
+	_, err := tr.SubmitContext(ctx, op)
+	require.NoError(t, err)
+
+	assert.EqualT(t, 1, mock.submitContextCalls, "wrapper must call SubmitContext on a ContextualTransport")
+	assert.EqualT(t, 0, mock.submitCalls, "legacy Submit must not be called when SubmitContext is available")
+	assert.Equal(t, ctx, mock.lastSubmitCtx, "ctx must be forwarded verbatim")
+	assert.Equal(t, staleCtx, mock.lastOpCtx, "op.Context must not be mutated under the SubmitContext path")
+	assert.Len(t, exporter.GetSpans(), 1)
+}
+
+// Test_OpenTelemetryRuntime_submitContext_legacyFallback verifies the
+// fallback path when the wrapped transport implements only the legacy
+// Submit method: the wrapper stamps ctx onto op.Context for the call
+// and restores the prior value afterwards.
+func Test_OpenTelemetryRuntime_submitContext_legacyFallback(t *testing.T) {
+	exporter := tracetest.NewInMemoryExporter()
+	tp := tracesdk.NewTracerProvider(
+		tracesdk.WithSampler(tracesdk.AlwaysSample()),
+		tracesdk.WithSyncer(exporter),
+	)
+	otel.SetTracerProvider(tp)
+
+	tracer := tp.Tracer("go-runtime")
+	ctx, span := tracer.Start(context.Background(), "op")
+	defer span.End()
+
+	header := map[string][]string{}
+	mock := &mockRuntime{req: runtime.TestClientRequest{Headers: header}}
+	tr := newOpenTelemetryTransport(mock, "remote_host", nil)
+
+	prevCtx, prevSpan := tracer.Start(context.Background(), "prev")
+	defer prevSpan.End()
+	op := testOperation(prevCtx)
+	_, err := tr.SubmitContext(ctx, op)
+	require.NoError(t, err)
+
+	assert.Equal(t, prevCtx, op.Context, "op.Context must be restored after the wrapped Submit returns")
+	assert.Len(t, exporter.GetSpans(), 1)
+}
+
 func assertOpenTelemetrySubmit(t *testing.T, operation *runtime.ClientOperation, exporter *tracetest.InMemoryExporter, expectedSpanCount int) {
 	header := map[string][]string{}
 	tr := newOpenTelemetryTransport(&mockRuntime{runtime.TestClientRequest{Headers: header}}, "remote_host", nil)

client/runtime.go

@@ -188,6 +188,22 @@ func (r *Runtime) CreateHttpRequest(operation *runtime.ClientOperation) (req *ht
 	return
 }
 
+// ContextualTransport extends [runtime.ClientTransport] with an
+// explicit context-aware submission method.
+//
+// [Runtime] satisfies it via [Runtime.SubmitContext]. Wrappers such
+// as the OpenTelemetry transport type-assert to this interface so
+// they can forward an explicit context to the underlying transport
+// without setting the cached [runtime.ClientOperation.Context] field.
+//
+// In v2, SubmitContext will be folded into [runtime.ClientTransport]
+// itself and the cached context field removed; this interface is the
+// v0.x bridge.
+type ContextualTransport interface {
+	runtime.ClientTransport
+	SubmitContext(context.Context, *runtime.ClientOperation) (any, error)
+}
+
 // Submit a request and when there is a body on success it will turn that into the result
 // all other things are turned into an api error for swagger which retains the status code.
 //

form.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"mime/multipart"
 	"net/http"
+	"strings"
 
 	"github.com/go-openapi/errors"
 )
@@ -281,8 +282,55 @@ func countFileParts(r *http.Request) int {
 	return n
 }
 
+// FormFile resolves a file field from a parsed form body, transparently
+// handling both content types accepted for `type: file` parameters by
+// the OpenAPI 2.0 spec:
+//
+//   - multipart/form-data — delegates to [http.Request.FormFile].
+//   - application/x-www-form-urlencoded — looks up the field in
+//     r.PostForm and synthesizes a [multipart.File] backed by the
+//     value bytes plus a [multipart.FileHeader] with Filename equal
+//     to the field name and Size set to the byte length.
+//
+// Returns [http.ErrMissingFile] when the field is absent under either
+// content type. Callers must have parsed the body upstream (e.g. via
+// [BindForm] or [http.Request.ParseForm]) before reading from the
+// urlencoded path — [http.Request.FormFile] takes care of parsing on
+// the multipart path.
+//
+// Presence is the only criterion for binding a urlencoded file: an
+// empty value (e.g. `file=`) is bound as a zero-byte file.
+func FormFile(r *http.Request, name string) (multipart.File, *multipart.FileHeader, error) {
+	file, header, err := r.FormFile(name)
+	if err == nil {
+		return file, header, nil
+	}
+	if !stderrors.Is(err, http.ErrNotMultipart) {
+		return nil, nil, err
+	}
+
+	values, present := r.PostForm[name]
+	if !present {
+		return nil, nil, http.ErrMissingFile
+	}
+	value := values[0]
+	return urlencodedFile{Reader: strings.NewReader(value)},
+		&multipart.FileHeader{Filename: name, Size: int64(len(value))},
+		nil
+}
+
+// urlencodedFile adapts a urlencoded form value (already buffered in
+// memory by [http.Request.ParseForm]) to the [multipart.File]
+// interface. The embedded [strings.Reader] supplies Read/ReadAt/Seek;
+// Close is a no-op since there is no resource to release.
+type urlencodedFile struct {
+	*strings.Reader
+}
+
+func (urlencodedFile) Close() error { return nil }
+
 func bindFormFile(r *http.Request, spec formFileSpec, maxFilenameLen int) error {
-	file, header, err := r.FormFile(spec.name)
+	file, header, err := FormFile(r, spec.name)
 	if err != nil {
 		if stderrors.Is(err, http.ErrMissingFile) {
 			if spec.required {