-
Notifications
You must be signed in to change notification settings - Fork 2
dynamodb: verify the admin PutItem URL key against the table schema #1229
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -312,7 +312,13 @@ func (d *DynamoDBServer) adminLoadReadableSchema(ctx context.Context, principal | |
| // - ErrAdminNotLeader — follower | ||
| // - ErrAdminDynamoNotFound — table absent | ||
| // - ErrAdminDynamoValidation — empty / malformed input | ||
| func (d *DynamoDBServer) AdminPutItem(ctx context.Context, principal AdminPrincipal, tableName string, item AdminItem) error { | ||
| func (d *DynamoDBServer) AdminPutItem( | ||
| ctx context.Context, | ||
| principal AdminPrincipal, | ||
| tableName string, | ||
| pathKey map[string]AdminAttributeValue, | ||
| item AdminItem, | ||
| ) error { | ||
| if !principal.Role.canWrite() { | ||
| return ErrAdminForbidden | ||
| } | ||
|
|
@@ -331,6 +337,9 @@ func (d *DynamoDBServer) AdminPutItem(ctx context.Context, principal AdminPrinci | |
| if err := validateAdminAttributeMapKinds(item.Attributes); err != nil { | ||
| return err | ||
| } | ||
| if err := d.assertPathKeyCoversPrimaryKey(ctx, tableName, pathKey); err != nil { | ||
| return err | ||
| } | ||
| in := putItemInput{ | ||
| TableName: tableName, | ||
| Item: adminToInternalAttributeMap(item.Attributes), | ||
|
|
@@ -341,6 +350,52 @@ func (d *DynamoDBServer) AdminPutItem(ctx context.Context, principal AdminPrinci | |
| return nil | ||
| } | ||
|
|
||
| // assertPathKeyCoversPrimaryKey rejects a PUT whose URL key does not | ||
| // name every primary-key attribute the table's schema declares. | ||
| // | ||
| // The HTTP layer already checks that each attribute IT received in the | ||
| // URL is present in the body with the same value, but it has no schema | ||
| // access — so it cannot tell that a composite-key table's URL segment | ||
| // carried only the hash key while the body supplied the range key. The | ||
| // write would then land on a row the URL never fully identified, which | ||
| // is the difference between "update the item this URL names" and | ||
| // "create some other item". | ||
| // | ||
| // Only the adapter can make that judgement, which is why the URL key | ||
| // is plumbed down here rather than validated above. | ||
| func (d *DynamoDBServer) assertPathKeyCoversPrimaryKey( | ||
| ctx context.Context, tableName string, pathKey map[string]AdminAttributeValue, | ||
| ) error { | ||
| schema, exists, err := d.loadTableSchema(ctx, tableName) | ||
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| if !exists { | ||
| return ErrAdminDynamoNotFound | ||
| } | ||
|
|
||
| required := []string{schema.PrimaryKey.HashKey} | ||
| if schema.PrimaryKey.RangeKey != "" { | ||
| required = append(required, schema.PrimaryKey.RangeKey) | ||
| } | ||
| for _, name := range required { | ||
| if _, ok := pathKey[name]; !ok { | ||
| return errors.Wrapf(ErrAdminDynamoValidation, | ||
| "path key is missing primary key attribute %q", name) | ||
| } | ||
| } | ||
| // Reject extras too: a URL key carrying an attribute the schema | ||
| // does not treat as part of the primary key means the caller and | ||
| // the table disagree about identity, and silently ignoring it | ||
| // would let the mismatch through. | ||
| if len(pathKey) != len(required) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For an existing table whose HASH and RANGE entries use the same attribute name, this comparison rejects every PUT because Useful? React with 👍 / 👎. |
||
| return errors.Wrapf(ErrAdminDynamoValidation, | ||
| "path key declares %d attributes but the primary key has %d", | ||
| len(pathKey), len(required)) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // AdminDeleteItem removes one item by primary key. Write role | ||
| // required. | ||
| // | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the table is deleted and recreated with a different primary-key schema after this check,
putItemWithRetryreloads the replacement schema inpreparePutItemWriteand its generation-fence retry path, but never reruns this path-key validation. If the body contains attributes for both schemas, the PUT can therefore pass using the old schema and then successfully write an item keyed by attributes absent from the URL under the new schema, preserving the wrong-resource write this change is intended to prevent. Perform the check inside each write preparation using the same schema that constructs the item key.Useful? React with 👍 / 👎.