From: | Noah Misch <noah(at)leadboat(dot)com> |
---|---|
To: | Stephen Frost <sfrost(at)snowman(dot)net> |
Cc: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: WITH CHECK and Column-Level Privileges |
Date: | 2015-01-14 06:59:26 |
Message-ID: | 20150114065926.GB2867136@tornado.leadboat.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Jan 12, 2015 at 05:16:40PM -0500, Stephen Frost wrote:
> Alright, here's an updated patch which doesn't return any detail if no
> values are visible or if only a partial key is visible.
I browsed this patch. There's been no mention of foreign key constraints, but
ri_ReportViolation() deserves similar hardening. If a user has only DELETE
privilege on a PK table, FK violation messages should not leak the PK values.
Modifications to the foreign side are less concerning, since the user will
often know the attempted value; still, I would lock down both sides.
Please add a comment explaining the safety of each row-emitting message you
haven't changed. For example, the one in refresh_by_match_merge() is safe
because getting there requires MV ownership.
> --- a/src/backend/access/nbtree/nbtinsert.c
> +++ b/src/backend/access/nbtree/nbtinsert.c
> @@ -388,18 +388,30 @@ _bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
> {
> Datum values[INDEX_MAX_KEYS];
> bool isnull[INDEX_MAX_KEYS];
> + char *key_desc;
>
> index_deform_tuple(itup, RelationGetDescr(rel),
> values, isnull);
> - ereport(ERROR,
> - (errcode(ERRCODE_UNIQUE_VIOLATION),
> - errmsg("duplicate key value violates unique constraint \"%s\"",
> - RelationGetRelationName(rel)),
> - errdetail("Key %s already exists.",
> - BuildIndexValueDescription(rel,
> - values, isnull)),
> - errtableconstraint(heapRel,
> - RelationGetRelationName(rel))));
> +
> + key_desc = BuildIndexValueDescription(rel, values,
> + isnull);
> +
> + if (!strlen(key_desc))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNIQUE_VIOLATION),
> + errmsg("duplicate key value violates unique constraint \"%s\"",
> + RelationGetRelationName(rel)),
> + errtableconstraint(heapRel,
> + RelationGetRelationName(rel))));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_UNIQUE_VIOLATION),
> + errmsg("duplicate key value violates unique constraint \"%s\"",
> + RelationGetRelationName(rel)),
> + errdetail("Key %s already exists.",
> + key_desc),
> + errtableconstraint(heapRel,
> + RelationGetRelationName(rel))));
Instead of duplicating an entire ereport() to change whether you include an
errdetail, use "condition ? errdetail(...) : 0".
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2015-01-14 07:48:00 | Re: PATCH: Reducing lock strength of trigger and foreign key DDL |
Previous Message | Robert Haas | 2015-01-14 04:28:35 | Re: Transactions involving multiple postgres foreign servers |