Skip to content

Commit

Permalink
Improved attribute validation (#109)
Browse files Browse the repository at this point in the history 
* fix numeric attribute decoding

* fix attribute test

* add type checks in setattr

* fix remcli set attribute

* fix remcli

* fix attribute tests

* add Double type

* update attribute test

* fixed tests

* fixed tests

* updated test
  • Loading branch information
@me-vadim @roman-tik
me-vadim authored and roman-tik committed Dec 13, 2019
1 parent 2df66d8 commit 5108e85
Show file tree
Hide file tree
Showing 7 changed files with 125 additions and 73 deletions.
2 changes: 1 addition & 1 deletion contracts/contracts/rem.attr/include/rem.attr/rem.attr.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
#include <eosio/eosio.hpp>

namespace eosio {

struct [[eosio::table, eosio::contract("rem.auth")]] attribute_info {
name attribute_name;
int32_t type;
Expand Down Expand Up @@ -84,6 +83,7 @@ namespace eosio {
enum class data_type : int32_t { Boolean = 0, Int, LargeInt, Double, ChainAccount, UTFString, DateTimeUTC, CID, OID, Binary, Set, MaxVal };
enum class privacy_type : int32_t { SelfAssigned = 0, PublicPointer, PublicConfirmedPointer, PrivatePointer, PrivateConfirmedPointer, MaxVal };

void check_attribute_data(const std::vector<char>& data, int32_t type) const;
void check_permission(const name& issuer, const name& receiver, int32_t ptype) const;
bool need_confirm(int32_t ptype) const;
};
Expand Down
38 changes: 37 additions & 1 deletion contracts/contracts/rem.attr/src/rem.attr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,13 @@ namespace eosio {
{
require_auth( issuer );
require_recipient( receiver );
check( !value.empty(), "value is empty" );

attribute_info_table attributes_info( _self, _self.value );
const auto& attrinfo = attributes_info.get( attribute_name.value, "attribute does not exist" );
check(attrinfo.next_id < std::numeric_limits<uint64_t>::max(), "attribute storage is full");
check( attrinfo.is_valid(), "this attribute is beeing deleted" );
check_permission(issuer, receiver, attrinfo.ptype);
check_attribute_data(value, attrinfo.type);

const auto id = attrinfo.next_id;

Expand Down Expand Up @@ -137,6 +137,42 @@ namespace eosio {
});
}

void attribute::check_attribute_data(const std::vector<char>& data, int32_t type) const
{
check( !data.empty(), "value is empty" );
switch(static_cast<data_type>( type )) {
case data_type::Boolean:
check( data.size() == 1, "invalid Boolean value" );
break;
case data_type::Int:
check( data.size() == sizeof(int32_t), "invalid Int value" );
break;
case data_type::LargeInt:
check( data.size() == sizeof(int64_t), "invalid LargeInt value" );
break;
case data_type::ChainAccount:
check( data.size() == 40, "invalid ChainAccount value" );
break;
case data_type::UTFString:
check( data.size() == data.front() + 1, "invalid UTFString value" );
break;
case data_type::DateTimeUTC:
check( data.size() == sizeof(int64_t), "invalid DateTimeUTC value" );
break;
case data_type::Binary:
check( data.size() == data.front() + 1, "invalid Binary value" );
break;
case data_type::Double:
check( data.size() == sizeof(double), "invalid Double value" );
break;
case data_type::CID:
case data_type::OID:
case data_type::Set:
default:
break;
}
}

void attribute::check_permission(const name& issuer, const name& receiver, int32_t ptype) const
{
if (static_cast<privacy_type>(ptype) == privacy_type::SelfAssigned) {
Expand Down
106 changes: 52 additions & 54 deletions programs/remcli/attribute_helpers.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,65 +143,63 @@ std::string decodeAttribute(const std::string& hex, int32_t type)
fc::from_hex(hex, data.data(), data.size());
fc::variant v;

switch (type)
{
case 0:
v = fc::raw::unpack<bool>(data);
break;
case 1:
v = fc::raw::unpack<int32_t>(data);
break;
case 2:
v = fc::raw::unpack<int64_t>(data);
break;
case 3:
v = fc::raw::unpack<std::pair<fc::sha256, eosio::name>>(data);
break;
case 4:
v = fc::raw::unpack<std::string>(data);
break;
case 5:
v = fc::time_point(fc::seconds(fc::raw::unpack<int64_t>(data)));
break;
case 6:
if (data.size() == 34 && data[0] == 18 && data[1] == 32) {
auto codec = multibase::base_58_btc{};
auto view = std::string_view(data.data(), data.size());
auto encoded = codec.encode(view, false);
v = encoded;
if (type == 0) {
v = fc::raw::unpack<bool>(data);
}
else if (type == 1) {
v = fc::raw::unpack<int32_t>(data);
}
else if (type == 2) {
v = fc::raw::unpack<int64_t>(data);
}
else if (type == 3) {
v = fc::raw::unpack<std::pair<fc::sha256, eosio::name>>(data);
}
else if (type == 4) {
v = fc::raw::unpack<std::string>(data);
}
else if (type == 5) {
v = fc::time_point(fc::seconds(fc::raw::unpack<int64_t>(data)));
}
else if (type == 6) {
if (data.size() == 34 && data[0] == 18 && data[1] == 32) {
auto codec = multibase::base_58_btc{};
auto view = std::string_view(data.data(), data.size());
auto encoded = codec.encode(view, false);
v = encoded;
}
else {
int64_t varint = 0;
if (static_cast<unsigned char>(data[0]) <= 252) {
varint = static_cast<unsigned char>(data[0]);
}
else {
int64_t varint = 0;
if (static_cast<unsigned char>(data[0]) <= 252) {
varint = static_cast<unsigned char>(data[0]);
size_t size = static_cast<unsigned char>(data[0]) - 252;
for (size_t i = 0; i < size; i++) {
varint <<= 8;
varint &= static_cast<unsigned char>(data[i + 1]);
}
else {
size_t size = static_cast<unsigned char>(data[0]) - 252;
for (int i = 0; i < size; i++) {
varint <<= 8;
varint &= static_cast<unsigned char>(data[i + 1]);
}

}
if (varint == 1) {
auto codec = multibase::base_58_btc{};
auto view = std::string_view(data.data(), data.size());
auto encoded = codec.encode(view, true);
v = encoded;
}
}
break;
case 7:
v = decodeOID(data);
break;
case 8:
v = data;
break;
case 9:
v = fc::raw::unpack<std::set<std::pair<eosio::name, std::string>>>(data);
break;
default:
EOS_ASSERT( false, eosio::chain::unknown_attribute_type, "Unknown attribute type" );
if (varint == 1) {
auto codec = multibase::base_58_btc{};
auto view = std::string_view(data.data(), data.size());
auto encoded = codec.encode(view, true);
v = encoded;
}
}
}
else if (type == 7) {
v = decodeOID(data);
}
else if (type == 8) {
v = data;
}
else if (type == 9) {
v = fc::raw::unpack<std::set<std::pair<eosio::name, std::string>>>(data);
}
else {
EOS_ASSERT( false, eosio::chain::unknown_attribute_type, "Unknown attribute type" );
}
return fc::json::to_pretty_string(v);
}
Expand Down
2 changes: 2 additions & 0 deletions programs/remcli/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2575,6 +2575,8 @@ int main( int argc, char** argv ) {
setAttribute->add_option("receiver", setAttrReceiver, localized("The name of the account who recieves an attribute"))->required();
setAttribute->add_option("attribute_name", setAttrName, localized("The name of the attribute"))->required();
setAttribute->add_option("value", attrValue, localized("Value of the attribute"))->required();
add_standard_transaction_options(setAttribute, "issuer@active");

setAttribute->set_callback([&] {
auto result = call(get_table_func, fc::mutable_variant_object("json", true)
("code", name(config::attribute_account_name))
Expand Down
18 changes: 9 additions & 9 deletions unittests/gift_resources_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,9 @@ BOOST_FIXTURE_TEST_CASE(acc_creation_with_attr_set, gift_resources_tester)
BOOST_REQUIRE(acc_gifter_attr.privacy_type == attr_info["ptype"].as_int64());

// the length of hex string should be even number
// 100% = 1000000 in decimal = 40420f0000000000 in hex big-endian
set_attr(N(rem.attr), config::system_account_name, acc_gifter_attr_name, "40420f0000000000");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), config::system_account_name, acc_gifter_attr_name)["data"].as_string() == "40420f0000000000");
// 100% = 1000000 in decimal = 40420f00 in hex big-endian
set_attr(N(rem.attr), config::system_account_name, acc_gifter_attr_name, "40420f00");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), config::system_account_name, acc_gifter_attr_name)["data"].as_string() == "40420f00");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), config::system_account_name, acc_gifter_attr_name)["pending"].as_string().empty());
}

Expand Down Expand Up @@ -344,8 +344,8 @@ BOOST_FIXTURE_TEST_CASE(acc_creation_with_attr_set, gift_resources_tester)
transfer( config::system_account_name, N(testram11111), asset{ 10'000'0000 } );
BOOST_REQUIRE( get_balance(N(testram11111)) == asset{ 10'000'0000 } );

set_attr(N(rem.attr), N(testram11111), acc_gifter_attr_name, "40420f0000000000");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), N(testram11111), acc_gifter_attr_name)["data"].as_string() == "40420f0000000000");
set_attr(N(rem.attr), N(testram11111), acc_gifter_attr_name, "40420f00");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), N(testram11111), acc_gifter_attr_name)["data"].as_string() == "40420f00");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), N(testram11111), acc_gifter_attr_name)["pending"].as_string().empty());

create_account_with_resources(N(testram22222), N(testram11111), asset{min_account_stake}, false);
Expand All @@ -358,8 +358,8 @@ BOOST_FIXTURE_TEST_CASE(acc_creation_with_attr_set, gift_resources_tester)

// set `accgifter` attribute to 50% for `testram11111` so now it should pay 50% of min stake
{
set_attr(N(rem.attr), N(testram11111), acc_gifter_attr_name, "20a1070000000000");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), N(testram11111), acc_gifter_attr_name)["data"].as_string() == "20a1070000000000");
set_attr(N(rem.attr), N(testram11111), acc_gifter_attr_name, "20a10700");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), N(testram11111), acc_gifter_attr_name)["data"].as_string() == "20a10700");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), N(testram11111), acc_gifter_attr_name)["pending"].as_string().empty());

BOOST_REQUIRE_EXCEPTION(
Expand All @@ -377,8 +377,8 @@ BOOST_FIXTURE_TEST_CASE(acc_creation_with_attr_set, gift_resources_tester)

// set `accgifter` attribute to 20% for `testram11111` so now it should pay 80% of min stake
{
set_attr(N(rem.attr), N(testram11111), acc_gifter_attr_name, "400d030000000000");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), N(testram11111), acc_gifter_attr_name)["data"].as_string() == "400d030000000000");
set_attr(N(rem.attr), N(testram11111), acc_gifter_attr_name, "400d0300");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), N(testram11111), acc_gifter_attr_name)["data"].as_string() == "400d0300");
BOOST_REQUIRE(get_account_attribute(N(rem.attr), N(testram11111), acc_gifter_attr_name)["pending"].as_string().empty());

BOOST_REQUIRE_EXCEPTION(
Expand Down
31 changes: 23 additions & 8 deletions unittests/rem_attr_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ struct set_attr_expected_t : public set_attr_t {
virtual bool isValueOk(std::string actualValue) const {
bytes b(actualValue.size() / 2);
const auto size = fc::from_hex(actualValue, b.data(), b.size());

const auto v = fc::raw::unpack<T>(b);
return v == expectedValue;
}
Expand Down Expand Up @@ -313,6 +312,7 @@ BOOST_FIXTURE_TEST_CASE( attribute_test, attribute_tester ) {
{ .attr_name=N(creator), .type=0, .privacy_type=3 }, // type: Bool, privacy type: PrivatePointer
{ .attr_name=N(name), .type=5, .privacy_type=0 }, // type: UTFString, privacy type: SelfAssigned
{ .attr_name=N(largeint), .type=2, .privacy_type=2 }, // type: LargeInt, privacy type: PublicConfirmedPointer
{ .attr_name=N(floating), .type=3, .privacy_type=1 }, // type: Double, privacy type: PublicPointer
};
for (const auto& a: attributes) {
create_attr(a.attr_name, a.type, a.privacy_type);
Expand All @@ -323,11 +323,11 @@ BOOST_FIXTURE_TEST_CASE( attribute_test, attribute_tester ) {
}
BOOST_REQUIRE_THROW(base_tester::push_action(N(rem.attr), N(create), N(b1), mvo()("attribute_name", name(N(fail)))("type", 0)("ptype", 0)),
missing_auth_exception);
BOOST_REQUIRE_THROW(create_attr(N(fail), -1, 0), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(create_attr(N(fail), 11, 0), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(create_attr(N(fail), 0, -1), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(create_attr(N(fail), 0, 5), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(create_attr(N(tags), 0, 0), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(create_attr(N(fail), -1, 0), eosio_assert_message_exception); // type is out of range
BOOST_REQUIRE_THROW(create_attr(N(fail), 11, 0), eosio_assert_message_exception); // type is out of range
BOOST_REQUIRE_THROW(create_attr(N(fail), 0, -1), eosio_assert_message_exception); // privacy type is out of range
BOOST_REQUIRE_THROW(create_attr(N(fail), 0, 5), eosio_assert_message_exception); // privacy type is out of range
BOOST_REQUIRE_THROW(create_attr(N(tags), 0, 0), eosio_assert_message_exception); // attribute with this name already exists

set_attr_expected_t<std::pair<fc::sha256, name>> attr1(N(prodb), N(proda), N(crosschain),
// concatenated "00000000000000000000000000000000000000000000000000000000000000ff" and "rem" in hex
Expand All @@ -353,12 +353,15 @@ BOOST_FIXTURE_TEST_CASE( attribute_test, attribute_tester ) {
"06426c61697a65", // encoded string "Blaize"
"Blaize");
set_attr_expected_t<int64_t> attr8(N(proda), N(prodb), N(largeint),
"0000000000000080", // encoded number -9223372036854775808
"0000000000000080", // encoded number -9223372036854775808 (8000000000000000 reversed because of mechanics of pack/unpack)
-9223372036854775808);
set_attr_expected_t<int64_t> attr9(N(prodb), N(prodb), N(largeint),
"ffffffffffffffff", // encoded number -1
-1);
for (auto& attr: std::vector<std::reference_wrapper<set_attr_t>>{ attr1, attr2, attr3, attr4, attr5, attr6, attr7, attr8, attr9 }) {
set_attr_expected_t<double> attr10(N(prodb), N(prodb), N(floating),
"0000000000000040", // encoded number 2.0
2.0);
for (auto& attr: std::vector<std::reference_wrapper<set_attr_t>>{ attr1, attr2, attr3, attr4, attr5, attr6, attr7, attr8, attr9, attr10 }) {
auto& attr_ref = attr.get();
set_attr(attr_ref.issuer, attr_ref.receiver, attr_ref.attribute_name, attr_ref.value);

Expand Down Expand Up @@ -402,6 +405,8 @@ BOOST_FIXTURE_TEST_CASE( attribute_test, attribute_tester ) {
BOOST_TEST(get_account_attribute( N(prodb), N(proda), N(largeint)) ["pending"].as_string() != ""); //not confirmed
BOOST_TEST(get_account_attribute( N(prodb), N(prodb), N(largeint)) ["data"].as_string() == "");
BOOST_TEST(get_account_attribute( N(prodb), N(prodb), N(largeint)) ["pending"].as_string() != ""); //not confirmed
BOOST_TEST(get_account_attribute( N(prodb), N(prodb), N(floating)) ["data"].as_string() != "");
BOOST_TEST(get_account_attribute( N(prodb), N(prodb), N(floating)) ["pending"].as_string() == "");

// only proda is allowed to confirm his attributes
BOOST_REQUIRE_THROW(base_tester::push_action(N(rem.attr), N(confirm), N(rem.attr), mvo()("owner", "proda")("issuer", "prodb")("attribute_name", "tags")),
Expand Down Expand Up @@ -445,12 +450,16 @@ BOOST_FIXTURE_TEST_CASE( attribute_test, attribute_tester ) {
BOOST_REQUIRE_THROW(unset_attr(N(proda), N(proda), N(creator)), eosio_assert_message_exception); //only contract owner can unset
BOOST_REQUIRE_THROW(unset_attr(N(rem.attr), N(proda), N(name)), eosio_assert_message_exception); //only receiver can unset
BOOST_REQUIRE_THROW(unset_attr(N(prodc), N(prodb), N(largeint)), eosio_assert_message_exception); //only issuer or receiver can unset
BOOST_REQUIRE_THROW(unset_attr(N(proda), N(prodb), N(floating)), eosio_assert_message_exception); //only issuer can unset
BOOST_REQUIRE_THROW(unset_attr(N(prodc), N(prodb), N(floating)), eosio_assert_message_exception); //only issuer can unset

// Attributes can`t be removed until they are marked as invalid and all attributes that have been set are deleted
BOOST_REQUIRE_THROW(remove_attr(N(crosschain)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(tags)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(creator)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(name)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(largeint)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(floating)), eosio_assert_message_exception);

unset_attr(N(prodb), N(proda), N(crosschain));
unset_attr(N(rem.attr), N(proda), N(tags));
Expand All @@ -459,6 +468,7 @@ BOOST_FIXTURE_TEST_CASE( attribute_test, attribute_tester ) {
unset_attr(N(rem.attr), N(rem.attr), N(creator));
unset_attr(N(prodb), N(prodb), N(name));
unset_attr(N(prodb), N(prodb), N(largeint));
unset_attr(N(prodb), N(prodb), N(floating));
// receiver can unset PublicConfirmedPointer
base_tester::push_action(N(rem.attr), N(unsetattr), N(prodb), mvo()("issuer", "proda")("receiver", "prodb")("attribute_name", "largeint"));
produce_block();
Expand All @@ -470,24 +480,29 @@ BOOST_FIXTURE_TEST_CASE( attribute_test, attribute_tester ) {
BOOST_TEST(get_account_attribute( N(prodb), N(prodb), N(name)).is_null());
BOOST_TEST(get_account_attribute( N(prodb), N(prodb), N(largeint)).is_null());
BOOST_TEST(get_account_attribute( N(prodb), N(proda), N(largeint)).is_null());
BOOST_TEST(get_account_attribute( N(prodb), N(prodb), N(floating)).is_null());

// Attributes can`t be removed until they are marked as invalid
BOOST_REQUIRE_THROW(remove_attr(N(crosschain)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(tags)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(creator)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(name)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(largeint)), eosio_assert_message_exception);
BOOST_REQUIRE_THROW(remove_attr(N(floating)), eosio_assert_message_exception);

invalidate_attr(N(crosschain));
invalidate_attr(N(tags));
invalidate_attr(N(creator));
invalidate_attr(N(name));
invalidate_attr(N(largeint));
invalidate_attr(N(floating));

remove_attr(N(crosschain));
remove_attr(N(tags));
remove_attr(N(creator));
remove_attr(N(name));
remove_attr(N(largeint));
remove_attr(N(floating));

} FC_LOG_AND_RETHROW()
}
Expand Down
1 change: 1 addition & 0 deletions unittests/rem_auth_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,7 @@ BOOST_FIXTURE_TEST_CASE( addkeyacc_pay_by_rem_with_discount_test, rem_auth_teste
auto account_balance_before = get_balance(account);
auto auth_contract_balance_before = get_balance(N(rem.auth));

// attribute name, data_type::Double, privacy_type::PrivatePointer
create_attr(N(discount), 3, 3);
set_attr(N(rem.auth), account, N(discount), "d7a3703d0ad7eb3f"); // value = 0.87

Expand Down

0 comments on commit 5108e85

Please sign in to comment.