Fix: user-defined field aliases may break federation #94

gmac · 2021-11-06T19:43:58Z

This adds _bramble_id as a universal ID hint, which allows id to behave as a standard user-defined selection. This fixes the numerous ways to break federation with user-defined field aliases describe in #90 and #93.

Apologies for the large PR – the vast majority of it is test fixtures.

The Problem

As described in #90, there are many creative ways for user-defined queries to break Bramble federation ids using field aliases, for example:

query {
  shop1 {
    products {
      boo: id
      name
    }
  }
}

While dynamic adjustments can be made to the query planner to account for such aliases, a user-aliased id still ends up breaking in the execution resolver, per #93.

A general observation: query planning currently optimizes for vanity queries at the cost of tricky logic holes and extra looping. The process becomes simpler and more reliable when the query planner always adds consistent implementation fields for itself, even if they are redundant with the contents of the user-defined query. Not making id do double-duty as both a user selection and an implementation detail avoids contention between the planning and resolution steps.

The fix(es)

Makes the planner always add a _bramble_id: id selection to boundary scopes. This becomes a universal implementation key, eliminating _id and leaving id untouched as a basic user selection.
Validates _bramble_id and __typename aliases. A user selection will not be allowed to hijack these fields.
Foregoes vanity queries in favor of eliminating childstep recursion complexity and selectionSetHasFieldNamed looping. Queries are now more verbose, but they are extremely consistent and avoid suspected logic holes.

Duplicated selections are ugly

If there’s human sensitivity to duplicated field selections (machines don’t care), then I’d propose adding a single-pass loop at the end of extractSelectionSet that imposes uniqueness once based on field alias falling back to field name. At present, “selectionSetHasFieldNamed” runs for each de-duplicated field, and doesn’t take into account field aliases.

Resolves #90.
Resolves #93.

codecov-commenter · 2021-11-06T19:44:51Z

Codecov Report

Merging #94 (9713919) into main (62b21ae) will increase coverage by 0.25%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
+ Coverage   67.15%   67.41%   +0.25%     
==========================================
  Files          24       24              
  Lines        2749     2731      -18     
==========================================
- Hits         1846     1841       -5     
+ Misses        748      740       -8     
+ Partials      155      150       -5

Impacted Files	Coverage Δ
query_execution.go	`69.70% <66.66%> (-0.19%)`	⬇️
plan.go	`83.11% <100.00%> (+1.07%)`	⬆️
auth.go	`86.79% <0.00%> (-0.63%)`	⬇️
execution.go	`76.53% <0.00%> (+1.36%)`	⬆️
introspection.go	`80.00% <0.00%> (+11.03%)`	⬆️

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 62b21ae...9713919. Read the comment docs.

gmac · 2021-11-08T01:37:45Z

plan_test.go

@@ -844,11 +818,19 @@ func TestQueryPlanNoUnnessecaryID(t *testing.T) {
 		  {
 			"ServiceURL": "A",
 			"ParentType": "Query",
-			"SelectionSet": "{ movies { title } }",
+			"SelectionSet": "{ movies { title _bramble_id: id } }",


I believe this test here represented another broken scenario: the boundary type (Movie) was being requested without an id. Digging into the planning flow, the omission was caused by a nuance in the childstep tracking. I've eliminated the childstep logic entirely as its sole purpose appears to have been controlling selection redundancies.

gmac · 2021-11-08T01:44:26Z

plan.go

@@ -66,7 +67,7 @@ func Plan(ctx *PlanningContext) (*QueryPlan, error) {
 		return nil, fmt.Errorf("not implemented")
 	}

-	steps, err := createSteps(ctx, nil, parentType, "", ctx.Operation.SelectionSet, false)
+	steps, err := createSteps(ctx, nil, parentType, "", ctx.Operation.SelectionSet)


I've eliminated the childstep recursion flag because its only purpose was to conditionally manage the inclusion of federation id's – to which I suspect it was buggy about. Now that _bramble_id is always included in boundary scopes, the complexity of childstep can be removed.

gmac · 2021-11-08T01:48:06Z

plan.go

+				if selection.Alias == reservedAlias && selection.Name != requiredName {
+					return nil, nil, gqlerror.Errorf("%s.%s: alias \"%s\" is reserved for system use", strings.Join(insertionPoint, "."), reservedAlias, reservedAlias)
+				}
+			}


This loop and the reservedAliases struct above block the submission of reserved aliases. If the user attempts to make a selection that will interfere with internal Bramble operations, we'll simply tell them it's not allowed.

gmac · 2021-11-08T01:54:22Z

plan.go

-			if !selectionSetHasFieldNamed(selectionSet, "__typename") {
-				selectionSet = append(selectionSet, &ast.Field{Alias: "__typename", Name: "__typename", Definition: &ast.FieldDefinition{Name: "__typename", Type: ast.NamedType("String", nil)}})
-			}
+			selectionSet = append(selectionSet, &ast.Field{Alias: "__typename", Name: "__typename", Definition: &ast.FieldDefinition{Name: "__typename", Type: ast.NamedType("String", nil)}})


The selectionSetHasFieldNamed function was buggy because it didn't account for field aliases. Rather than expanding its implementation to handle more complex naming considerations, I've opted to remove it entirely given that it was adding loops mostly for the sake of vanity selections. A __typename field will now always be appended to fragments, even if it was also included in the user-defined selection. While potentially more verbose, it makes planning results more predictable.

gmac · 2021-11-08T02:00:54Z

plan.go

-		id := &ast.Field{
-			Alias:      "_id",
-			Name:       "id",
-			Definition: parentDef.Fields.ForName("id"),


This check has been greatly simplified – any boundary object with an id definition gets a _bramble_id hint added to its selection set. It's more verbose, but it's extremely predictable and consistent.

gmac · 2021-11-08T02:04:28Z

plan.go

+var reservedAliases = map[string]string{
+	"__typename": "__typename",
+	"_bramble_id": "id",
+}


This is a mapping of reserved aliases – or, alias/name pairs that are sensitive to the implementation, and therefore will be validated on the gateway for integrity.

gmac · 2021-11-08T02:07:00Z

plan.go

+						SelectionSet: []ast.Selection{&ast.Field{Alias: "_bramble_id", Name: "id", Definition: idDef}},
+						ObjectDefinition: implementationType,
+					}
+					selectionSetResult = append(selectionSetResult, possibleId)


This adds the best-practice from below where the type is checked for an ID definition – I suppose the wildcard here is a namespace, which I assume is just a merged scope without an id?

gmac · 2021-11-08T02:09:01Z

plan_fixtures_test.go

@@ -264,7 +264,7 @@ var PlanTestFixture6 = &PlanTestFixture{
 }


-func (f *PlanTestFixture) Plan(t *testing.T, query string) *QueryPlan {
+func (f *PlanTestFixture) Plan(t *testing.T, query string) (*QueryPlan, error) {


I've split these test operations apart a bit more to add an additional check for error outcomes.

gmac · 2021-11-08T02:10:39Z

plan_test.go

@@ -357,32 +357,6 @@ func TestQueryPlanFragmentSpread1(t *testing.T) {
 	PlanTestFixture1.Check(t, query, plan)
 }

-func TestQueryPlanFragmentSpread1DontDuplicateTypename(t *testing.T) {


This test is no longer applicable now that __typename is, by design, always appended and allowed to repeat a user-defined selection.

gmac · 2021-11-08T02:14:18Z

query_execution.go

@@ -520,7 +520,7 @@ func mergeExecutionResultsRec(src interface{}, dst interface{}, insertionPoint [
 					}
 					if srcID == dstID {
 						for k, v := range result {
-							if k == "_id" || k == "id" {
+							if k == "_bramble_id" {


Then here's where #93 was breaking... this special case for the id field prevented it from resolving standard GraphQL aliases. Now that everything revolves around _bramble_id, we can leave id alone an allow it to resolve as a normal field.

gmac · 2021-11-08T02:51:43Z

Okay, ready for review here @nmaquet. Sorry for the large PR.

lucianjon · 2021-11-08T03:24:13Z

@gmac Thanks again for this one, have just had a skim so far but looks solid. One thought, if we're going the route of reserving the __typename and _bramble_id aliases, as well as guaranteeing the injection of __typename. What do you think about following the same pattern as the id field and making it something like _bramble__typename, this could also allow us to drop it from the final response like we do the id.

It would require updating the execution pipeline to look for that alias over the regular __typename but would have the benefit of clearly indicating it's been injected for the internal purposes of bramble.

gmac · 2021-11-08T03:28:42Z

@lucianjon — a dedicated typename alias makes sense to me. It’d certainly be ideal to keep __typename itself as native GraphQL to be abused in any way the end user likes.

lucianjon · 2021-11-08T03:45:48Z

@lucianjon — a dedicated typename alias makes sense to me. It’d certainly be ideal to keep __typename itself as native GraphQL to be abused in any way the end user likes.

Sweet, I think we could do it in another PR to keep the size of this one down.

lucianjon

Looks good from my perspective, will give someone else the chance to have a look though.

lucianjon · 2021-11-08T03:55:46Z

plan.go

+				if idDef := implementationType.Fields.ForName("id"); idDef != nil {
+					possibleId := &ast.InlineFragment{
+						TypeCondition: implementationName,
+						SelectionSet: []ast.Selection{&ast.Field{Alias: "_bramble_id", Name: "id", Definition: idDef}},


Could pay to pull _bramble_id out into a const since it's used in a few spots.

nmaquet · 2021-11-08T05:55:36Z

Thanks @gmac!!!

nmaquet marked this pull request as ready for review November 6, 2021 21:14

nmaquet requested a review from a team as a code owner November 6, 2021 21:14

gmac mentioned this pull request Nov 7, 2021

Broken boundary when aliasing id field #93

Closed

gmac marked this pull request as draft November 8, 2021 01:00

gmac commented Nov 8, 2021

View reviewed changes

gmac changed the title ~~Fix: prevent broken user-defined queries with a reserved ID alias~~ Fix: user-defined field aliases may break federation Nov 8, 2021

add _bramble_id as a universal implementation hint.

971b1f9

gmac force-pushed the add-validate-id-hints branch from 9713919 to 971b1f9 Compare November 8, 2021 02:50

gmac marked this pull request as ready for review November 8, 2021 02:50

gmac mentioned this pull request Nov 8, 2021

Always add __typename to abstract types #91

Merged

lucianjon approved these changes Nov 8, 2021

View reviewed changes

nmaquet merged commit e7fe007 into movio:main Nov 8, 2021

nmaquet mentioned this pull request Nov 8, 2021

Change reserved alias from ___typename to _bramble_typename #100

Closed

gmac deleted the add-validate-id-hints branch November 8, 2021 18:57

lucianjon mentioned this pull request Nov 9, 2021

Use _bramble__typename alias for bramble injected __typename #105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: user-defined field aliases may break federation #94

Fix: user-defined field aliases may break federation #94

gmac commented Nov 6, 2021 •

edited

Loading

codecov-commenter commented Nov 6, 2021 •

edited

Loading

gmac Nov 8, 2021

gmac Nov 8, 2021 •

edited

Loading

gmac Nov 8, 2021

gmac Nov 8, 2021 •

edited

Loading

gmac Nov 8, 2021 •

edited

Loading

gmac Nov 8, 2021

gmac Nov 8, 2021 •

edited

Loading

gmac Nov 8, 2021 •

edited

Loading

gmac Nov 8, 2021

gmac Nov 8, 2021

gmac commented Nov 8, 2021

lucianjon commented Nov 8, 2021

gmac commented Nov 8, 2021

lucianjon commented Nov 8, 2021

lucianjon left a comment

lucianjon Nov 8, 2021

nmaquet commented Nov 8, 2021

Fix: user-defined field aliases may break federation #94

Fix: user-defined field aliases may break federation #94

Conversation

gmac commented Nov 6, 2021 • edited Loading

The Problem

The fix(es)

Duplicated selections are ugly

codecov-commenter commented Nov 6, 2021 • edited Loading

Codecov Report

gmac Nov 8, 2021

Choose a reason for hiding this comment

gmac Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

gmac Nov 8, 2021

Choose a reason for hiding this comment

gmac Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

gmac Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

gmac Nov 8, 2021

Choose a reason for hiding this comment

gmac Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

gmac Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

gmac Nov 8, 2021

Choose a reason for hiding this comment

gmac Nov 8, 2021

Choose a reason for hiding this comment

gmac commented Nov 8, 2021

lucianjon commented Nov 8, 2021

gmac commented Nov 8, 2021

lucianjon commented Nov 8, 2021

lucianjon left a comment

Choose a reason for hiding this comment

lucianjon Nov 8, 2021

Choose a reason for hiding this comment

nmaquet commented Nov 8, 2021

gmac commented Nov 6, 2021 •

edited

Loading

codecov-commenter commented Nov 6, 2021 •

edited

Loading

gmac Nov 8, 2021 •

edited

Loading

gmac Nov 8, 2021 •

edited

Loading

gmac Nov 8, 2021 •

edited

Loading

gmac Nov 8, 2021 •

edited

Loading

gmac Nov 8, 2021 •

edited

Loading