From f20c3d73d20fcce1344ee1a44bfa6d7f8c35471f Mon Sep 17 00:00:00 2001 From: Bryan Frimin Date: Mon, 6 Jul 2026 10:13:06 +0200 Subject: [PATCH] Verify signature ownership in esign accept/record-event flows Any self-provisioned trust center visitor could accept another visitor's NDA signature or inject audit-trail events into it by supplying its GID, since AcceptSignature and RecordEvent trusted the client-supplied signature ID without checking it belonged to the caller (GHSA-22xj-f767-ppw6). SignerEmail/ActorEmail are always derived from the verified session identity, never client input, so comparing them against the signature's stored SignerEmail in pkg/esign/service.go closes the hole at its root without touching the resolver-level authorization already in place elsewhere. Adds an e2e regression test that self-provisions two trust center visitors through the real magic-link flow and confirms one cannot touch the other's signature. Signed-off-by: Bryan Frimin --- e2e/internal/testutil/client.go | 75 +++++- e2e/trust/main_test.go | 31 +++ e2e/trust/trust_center_nda_signature_test.go | 233 +++++++++++++++++++ pkg/esign/errors.go | 1 + pkg/esign/service.go | 9 + pkg/server/api/trust/v1/nda_resolvers.go | 8 + 6 files changed, 352 insertions(+), 5 deletions(-) create mode 100644 e2e/trust/main_test.go create mode 100644 e2e/trust/trust_center_nda_signature_test.go diff --git a/e2e/internal/testutil/client.go b/e2e/internal/testutil/client.go index 015908a63..21dd39a95 100644 --- a/e2e/internal/testutil/client.go +++ b/e2e/internal/testutil/client.go @@ -400,10 +400,16 @@ func (c *Client) inviteUser(profileID gid.GID) { } func (c *Client) getActivationToken(email string) string { + return c.pollForLinkToken(fmt.Sprintf("to:%s subject:\"Invitation to join\"", email)) +} + +// pollForLinkToken polls mailpit for a message matching searchQuery and +// returns the first "token" query parameter found among its links. +func (c *Client) pollForLinkToken(searchQuery string) string { deadline := time.Now().Add(10 * time.Second) for time.Now().Before(deadline) { - searchMails, err := c.SearchMails(fmt.Sprintf("to:%s subject:\"Invitation to join\"", email)) + searchMails, err := c.SearchMails(searchQuery) require.NoError(c.T, err, "mailpit messages search failed") for _, msg := range searchMails.Messages { @@ -414,9 +420,8 @@ func (c *Client) getActivationToken(email string) string { linkURL, err := url.Parse(link.URL) require.NoError(c.T, err, "mailpit link invalid URL") - query := linkURL.Query() - if query.Get("token") != "" { - return query.Get("token") + if token := linkURL.Query().Get("token"); token != "" { + return token } } } @@ -424,7 +429,7 @@ func (c *Client) getActivationToken(email string) string { time.Sleep(100 * time.Millisecond) } - c.T.Logf("activation token not found") + c.T.Logf("link token not found for query %q", searchQuery) c.T.FailNow() return "" @@ -530,6 +535,66 @@ func NewClientWithNewSession(t testing.TB, from *Client) *Client { return client } +// SelfProvisionTrustCenterVisitor signs a brand-new email up as a trust +// center visitor through the public magic-link flow (send + verify), the +// same path a real visitor takes. It returns a Client bound to that +// visitor's own session and cookie jar, scoped to no organization. +func SelfProvisionTrustCenterVisitor(t testing.TB, trustCenterID string) *Client { + t.Helper() + + jar, err := cookiejar.New(nil) + require.NoError(t, err, "cannot create cookie jar") + + email := fmt.Sprintf("visitor-%s@e2e.probo.test", generateUniqueID()) + + visitor := &Client{ + T: t, + baseURL: GetBaseURL(), + mailpitBaseURL: GetMailpitBaseURL(), + email: email, + httpClient: &http.Client{ + Jar: jar, + Timeout: 30 * time.Second, + }, + } + + visitor.sendMagicLink(trustCenterID, email) + token := visitor.pollForLinkToken(fmt.Sprintf("to:%s", email)) + visitor.verifyMagicLink(trustCenterID, token) + + return visitor +} + +func (c *Client) sendMagicLink(trustCenterID, email string) { + const query = ` + mutation($input: SendMagicLinkInput!) { + sendMagicLink(input: $input) { + success + } + } + ` + + err := c.ExecuteTrust(trustCenterID, query, map[string]any{ + "input": map[string]any{"email": email}, + }, nil) + require.NoError(c.T, err, "sendMagicLink mutation failed") +} + +func (c *Client) verifyMagicLink(trustCenterID, token string) { + const query = ` + mutation($input: VerifyMagicLinkInput!) { + verifyMagicLink(input: $input) { + continue + } + } + ` + + err := c.ExecuteTrust(trustCenterID, query, map[string]any{ + "input": map[string]any{"token": token}, + }, nil) + require.NoError(c.T, err, "verifyMagicLink mutation failed") +} + func (c *Client) GetEmail() string { return c.email } diff --git a/e2e/trust/main_test.go b/e2e/trust/main_test.go new file mode 100644 index 000000000..cb9752436 --- /dev/null +++ b/e2e/trust/main_test.go @@ -0,0 +1,31 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +// OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +package trust_test + +import ( + "os" + "testing" + + "go.probo.inc/probo/e2e/internal/testutil" +) + +func TestMain(m *testing.M) { + testutil.Setup() + + code := m.Run() + + testutil.Teardown() + os.Exit(code) +} diff --git a/e2e/trust/trust_center_nda_signature_test.go b/e2e/trust/trust_center_nda_signature_test.go new file mode 100644 index 000000000..7169c418d --- /dev/null +++ b/e2e/trust/trust_center_nda_signature_test.go @@ -0,0 +1,233 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +// OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +package trust_test + +import ( + "encoding/base64" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.probo.inc/probo/e2e/internal/testutil" +) + +// minimalPDFBase64 is a minimal but structurally valid one-page PDF (no +// visible content). Unlike a hand-rolled "%PDF-1.4 ... /Catalog" stub, it +// has a real Pages tree, so it survives the watermark stamping that +// ProvisionMember runs when creating a pending NDA signature. +const minimalPDFBase64 = "JVBERi0xLjcKJeLjz9MKMSAwIG9iago8PC9QYWdlcyAyIDAgUi9UeXBlL0NhdGFsb2c+PgplbmRv" + + "YmoKNCAwIG9iago8PC9GaWx0ZXIvRmxhdGVEZWNvZGUvTGVuZ3RoIDExPj4Kc3RyZWFtCnicAQAA" + + "//8AAAABZW5kc3RyZWFtCmVuZG9iagoyMyAwIG9iago8PC9GaWx0ZXIvRmxhdGVEZWNvZGUvRmly" + + "c3QgMTQvTGVuZ3RoIDE2Ni9OIDMvVHlwZS9PYmpTdG0+PgpzdHJlYW0KeJxczkHKwjAQBeCrzAn+" + + "Sdr+ugmzaEEEEUp1V7qI7SAFSaSZit5epi6UZhPem2/x/sFADtsCMrCZdQ6rGISDJCjAQIO1nzgI" + + "ZEtoOMV56jk5h7sYRD8Lud6IiPD8ujPW/spEzmHpE6vCPd8eLGPv8TRfRI1C++EqFl7FOQhYPIxD" + + "anVW0+GRh9GX8dmaP/PzYBULsyo2q6L7TktE7wAAAP//bYpDg2VuZHN0cmVhbQplbmRvYmoKNiAw" + + "IG9iago8PC9DcmVhdGlvbkRhdGUoRDoyMDE5MDcwNDEwMjcyOCswMicwMCcpL01vZERhdGUoRDoy" + + "MDE5MDcwNDEwMjcyOCswMicwMCcpL1Byb2R1Y2VyKHBkZmNwdSB2MC4xLjI1KT4+CmVuZG9iagoy" + + "MiAwIG9iago8PC9GaWx0ZXIvRmxhdGVEZWNvZGUvSURbPDEyNjNDMjQ4RDcyOUI5MTNGQzM5MkYy" + + "NjQ2MTk1NDJBPiA8MGNiOTUzNjAyNzk4NWQ1NTQ5YzNlY2ZlNmE5MjM5ZTc+XS9JbmRleFswIDIy" + + "IDIzIDFdL0luZm8gNiAwIFIvTGVuZ3RoIDcyL1Jvb3QgMSAwIFIvU2l6ZSAyNC9UeXBlL1hSZWYv" + + "V1sxIDIgMl0+PgpzdHJlYW0KeJwkzEkKgDAUBNH62TgbR7yMFxS8c6RMLx70puAsJciQuEgSwV0v" + + "ES//AhpppZNeBhllklmyLLLKJrsclh/4AgAA//9tzQSTZW5kc3RyZWFtCmVuZG9iagoKc3RhcnR4" + + "cmVmCjUxMgolJUVPRg==" + +// TestTrustCenter_AcceptElectronicSignature_RejectsForeignSignature is a +// regression test for GHSA-22xj-f767-ppw6: any self-provisioned trust +// center visitor could accept another visitor's NDA signature, or inject +// audit-trail events into it, simply by knowing its GID. +func TestTrustCenter_AcceptElectronicSignature_RejectsForeignSignature(t *testing.T) { + t.Parallel() + + owner := testutil.NewClient(t, testutil.RoleOwner) + trustCenterID := lookupTrustCenterID(t, owner) + + uploadTrustCenterNDA(t, owner, trustCenterID) + activateTrustCenter(t, owner, trustCenterID) + + victim := testutil.SelfProvisionTrustCenterVisitor(t, trustCenterID) + attacker := testutil.SelfProvisionTrustCenterVisitor(t, trustCenterID) + + victimSignatureID, victimSignatureStatus := viewerSignature(t, victim, trustCenterID) + require.NotEmpty(t, victimSignatureID) + require.Equal(t, "PENDING", victimSignatureStatus) + + const acceptMutation = ` + mutation($input: AcceptElectronicSignatureInput!) { + acceptElectronicSignature(input: $input) { + signature { id status } + } + } + ` + + err := attacker.ExecuteTrust(trustCenterID, acceptMutation, map[string]any{ + "input": map[string]any{"signatureId": victimSignatureID}, + }, nil) + require.Error(t, err, "attacker must not be able to accept another visitor's signature") + assertForbidden(t, err) + + const recordEventMutation = ` + mutation($input: RecordSigningEventInput!) { + recordSigningEvent(input: $input) { + success + } + } + ` + + err = attacker.ExecuteTrust(trustCenterID, recordEventMutation, map[string]any{ + "input": map[string]any{ + "signatureId": victimSignatureID, + "eventType": "DOCUMENT_VIEWED", + }, + }, nil) + require.Error(t, err, "attacker must not be able to inject events into another visitor's signature") + assertForbidden(t, err) + + _, statusAfterAttack := viewerSignature(t, victim, trustCenterID) + assert.Equal(t, "PENDING", statusAfterAttack, "attack attempts must not have mutated the victim's signature") + + var acceptResult struct { + AcceptElectronicSignature struct { + Signature struct { + ID string `json:"id"` + Status string `json:"status"` + } `json:"signature"` + } `json:"acceptElectronicSignature"` + } + + err = victim.ExecuteTrust(trustCenterID, acceptMutation, map[string]any{ + "input": map[string]any{"signatureId": victimSignatureID}, + }, &acceptResult) + require.NoError(t, err, "the legitimate signer must still be able to accept their own signature") + assert.Equal(t, "ACCEPTED", acceptResult.AcceptElectronicSignature.Signature.Status) +} + +func lookupTrustCenterID(t *testing.T, owner *testutil.Client) string { + t.Helper() + + const query = ` + query($organizationId: ID!) { + node(id: $organizationId) { + ... on Organization { + trustCenter { id } + } + } + } + ` + + var result struct { + Node struct { + TrustCenter struct { + ID string `json:"id"` + } `json:"trustCenter"` + } `json:"node"` + } + + err := owner.Execute(query, map[string]any{ + "organizationId": owner.GetOrganizationID().String(), + }, &result) + require.NoError(t, err) + require.NotEmpty(t, result.Node.TrustCenter.ID) + + return result.Node.TrustCenter.ID +} + +func uploadTrustCenterNDA(t *testing.T, owner *testutil.Client, trustCenterID string) { + t.Helper() + + const query = ` + mutation($input: UploadTrustCenterNDAInput!) { + uploadTrustCenterNDA(input: $input) { + trustCenter { id } + } + } + ` + + pdfContent, err := base64.StdEncoding.DecodeString(minimalPDFBase64) + require.NoError(t, err) + + err = owner.ExecuteWithFile(query, map[string]any{ + "input": map[string]any{ + "trustCenterId": trustCenterID, + "fileName": "nda.pdf", + "file": nil, + }, + }, "input.file", testutil.UploadFile{ + Filename: "nda.pdf", + ContentType: "application/pdf", + Content: pdfContent, + }, nil) + require.NoError(t, err) +} + +func activateTrustCenter(t *testing.T, owner *testutil.Client, trustCenterID string) { + t.Helper() + + const query = ` + mutation($input: UpdateTrustCenterInput!) { + updateTrustCenter(input: $input) { + trustCenter { id active } + } + } + ` + + err := owner.Execute(query, map[string]any{ + "input": map[string]any{ + "trustCenterId": trustCenterID, + "active": true, + }, + }, nil) + require.NoError(t, err) +} + +func viewerSignature(t *testing.T, visitor *testutil.Client, trustCenterID string) (string, string) { + t.Helper() + + const query = ` + query { + currentTrustCenter { + nonDisclosureAgreement { + viewerSignature { id status } + } + } + } + ` + + var result struct { + CurrentTrustCenter struct { + NonDisclosureAgreement struct { + ViewerSignature struct { + ID string `json:"id"` + Status string `json:"status"` + } `json:"viewerSignature"` + } `json:"nonDisclosureAgreement"` + } `json:"currentTrustCenter"` + } + + err := visitor.ExecuteTrust(trustCenterID, query, nil, &result) + require.NoError(t, err) + + sig := result.CurrentTrustCenter.NonDisclosureAgreement.ViewerSignature + + return sig.ID, sig.Status +} + +func assertForbidden(t *testing.T, err error) { + t.Helper() + + gqlErrs, ok := err.(testutil.GraphQLErrors) + require.True(t, ok, "expected a GraphQL error, got %T: %v", err, err) + require.NotEmpty(t, gqlErrs) + assert.Equal(t, "FORBIDDEN", gqlErrs[0].Code()) +} diff --git a/pkg/esign/errors.go b/pkg/esign/errors.go index da4dbec06..97fa6b9be 100644 --- a/pkg/esign/errors.go +++ b/pkg/esign/errors.go @@ -18,4 +18,5 @@ import "errors" var ( ErrElectronicSignatureNotFound = errors.New("electronic signature not found") + ErrSignatureAccessDenied = errors.New("signature does not belong to the caller") ) diff --git a/pkg/esign/service.go b/pkg/esign/service.go index f0ed111f9..99875ca52 100644 --- a/pkg/esign/service.go +++ b/pkg/esign/service.go @@ -19,6 +19,7 @@ import ( "context" "errors" "fmt" + "strings" "time" "go.gearno.de/crypto/uuid" @@ -327,6 +328,10 @@ func (s *Service) AcceptSignature(ctx context.Context, scope coredata.Scoper, re return fmt.Errorf("cannot load electronic signature: %w", err) } + if !strings.EqualFold(signature.SignerEmail, req.SignerEmail.String()) { + return ErrSignatureAccessDenied + } + if signature.Status != coredata.ElectronicSignatureStatusPending && signature.Status != coredata.ElectronicSignatureStatusFailed { return fmt.Errorf("cannot accept electronic signature in status %s", signature.Status) @@ -387,6 +392,10 @@ func (s *Service) RecordEvent(ctx context.Context, scope coredata.Scoper, req *R return fmt.Errorf("cannot load electronic signature: %w", err) } + if !strings.EqualFold(signature.SignerEmail, req.ActorEmail.String()) { + return ErrSignatureAccessDenied + } + return s.recordEvent(ctx, tx, scope, req) }, ) diff --git a/pkg/server/api/trust/v1/nda_resolvers.go b/pkg/server/api/trust/v1/nda_resolvers.go index 5c4165066..412affcf7 100644 --- a/pkg/server/api/trust/v1/nda_resolvers.go +++ b/pkg/server/api/trust/v1/nda_resolvers.go @@ -48,6 +48,10 @@ func (r *mutationResolver) AcceptElectronicSignature(ctx context.Context, input return nil, gqlutils.NotFoundf(ctx, "electronic signature %q not found", input.SignatureID) } + if errors.Is(err, esign.ErrSignatureAccessDenied) { + return nil, gqlutils.Forbiddenf(ctx, "cannot accept electronic signature") + } + r.logger.ErrorCtx(ctx, "cannot accept electronic signature", log.Error(err)) return nil, gqlutils.Internal(ctx) @@ -85,6 +89,10 @@ func (r *mutationResolver) RecordSigningEvent(ctx context.Context, input types.R return nil, gqlutils.NotFoundf(ctx, "electronic signature %q not found", input.SignatureID) } + if errors.Is(err, esign.ErrSignatureAccessDenied) { + return nil, gqlutils.Forbiddenf(ctx, "cannot record signing event") + } + r.logger.ErrorCtx(ctx, "cannot record signing event", log.Error(err)) return nil, gqlutils.Internal(ctx)